Added test_mypy.py and resolved all regressions in demo_nodes_py (#767)#780
Added test_mypy.py and resolved all regressions in demo_nodes_py (#767)#780cronenberg64 wants to merge 5 commits intoros2:rollingfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds ament_mypy linting to demo_nodes_py and updates the Python demo nodes/tests with type annotations to satisfy strict mypy checks, addressing issue #767.
Changes:
- Added a new
test_mypy.pytest and declaredament_mypyas a test dependency. - Added/expanded type annotations across multiple demo nodes (topics, services, parameters, logging, events) to resolve mypy regressions.
- Minor formatting tweaks (e.g., line wrapping) and added shebangs to several Python entrypoint modules.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| demo_nodes_py/test/test_pep257.py | Adds return type annotation to satisfy stricter typing expectations in tests. |
| demo_nodes_py/test/test_flake8.py | Adds return type annotation to satisfy stricter typing expectations in tests. |
| demo_nodes_py/test/test_copyright.py | Adds return type annotation to satisfy stricter typing expectations in tests. |
| demo_nodes_py/test/test_mypy.py | Introduces mypy linter test for the package. |
| demo_nodes_py/package.xml | Adds ament_mypy as a test_depend. |
| demo_nodes_py/demo_nodes_py/topics/talker.py | Adds shebang and type annotations (incl. timer type). |
| demo_nodes_py/demo_nodes_py/topics/talker_qos.py | Adds shebang and type annotations for QoS usage and timer. |
| demo_nodes_py/demo_nodes_py/topics/listener.py | Adds shebang, type annotations, and wraps subscription call for style. |
| demo_nodes_py/demo_nodes_py/topics/listener_qos.py | Adds shebang and type annotations for QoS usage and callbacks. |
| demo_nodes_py/demo_nodes_py/topics/listener_serialized.py | Adds shebang, callback typing for raw bytes subscription, and adjusts log formatting. |
| demo_nodes_py/demo_nodes_py/services/introspection.py | Adds shebang and type annotations (Timer/Future, callbacks, service handler). |
| demo_nodes_py/demo_nodes_py/services/add_two_ints_server.py | Adds shebang and type annotations for service handler. |
| demo_nodes_py/demo_nodes_py/services/add_two_ints_client.py | Adds shebang and type annotations; refactors result logging for mypy. |
| demo_nodes_py/demo_nodes_py/services/add_two_ints_client_async.py | Adds shebang and type annotations; refactors result logging for mypy. |
| demo_nodes_py/demo_nodes_py/parameters/set_parameters_callback.py | Adds shebang and type annotations for parameter callback hooks. |
| demo_nodes_py/demo_nodes_py/parameters/async_param_client.py | Adds shebang, type annotations, and renames futures/loop vars for clarity and typing. |
| demo_nodes_py/demo_nodes_py/logging/use_logger_service.py | Adds shebang and type annotations; adds a couple targeted mypy ignores for generated message fields. |
| demo_nodes_py/demo_nodes_py/events/matched_event_detect.py | Replaces ... lambdas with None returns and adds typing for callbacks/future helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
demo_nodes_py/test/test_mypy.py
Outdated
|
|
||
|
|
||
| def test_mypy() -> None: | ||
| rc = main() |
9f89306 to
ebdb446
Compare
…des_py (ros2#767) Resolves ros2#767 by enabling strict type checking. Key Changes: - Added strict annotations and return types to 14+ modules to pass mypy --strict. - Corrected a functional error in matched_event_detect.py where ROS message types were used as type annotations for topic names. - Added test/test_mypy.py and updated package.xml for ament_mypy support. - Ensured node scripts maintain correct #!/usr/bin/env python3 shebangs and LF line endings. Signed-off-by: Jonathan Setiawan <jonathanrustam2@gmail.com>
ebdb446 to
b639a2d
Compare
demo_nodes_py/demo_nodes_py/services/add_two_ints_client_async.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Jonathan Setiawan <jonathanrustam2@gmail.com>
|
@fujitatomoya @InvincibleRMC |
|
Pulls: #780 |
Signed-off-by: Jonathan Setiawan <jonathanrustam2@gmail.com>
Signed-off-by: Jonathan Setiawan <jonathanrustam2@gmail.com>
|
@InvincibleRMC Just a quick ping on this, I believe I've addressed all the previous feedback regarding the python ver compatibility, typing changes, and linter formatting issues. Let me know if there is anything else you need me to adjust, or if you are able to re-review and clear the requested changes. Thank you |
demo_nodes_py/test/test_mypy.py
Outdated
|
|
||
|
|
||
| def test_mypy() -> None: | ||
| rc = main() |
There was a problem hiding this comment.
| rc = main() | |
| rc = main('--ament-srict') |
There was a problem hiding this comment.
Thanks for the suggestion @InvincibleRMC, I actually tried adding this flag locally earlier, but it caused colcon test to fail for a couple of reasons. First, ament_mypy.main() expects a list of strings (argv=['...']), so passing a raw string causes argparse to fail as it tries to parse each character individually. Even when passed correctly as argv=['--ament-strict'] (fixing the slight typo), ament_mypy throws a SystemExit: 2 crash because --ament-strict isn't a recognized flag in the ament parser. If we want to enforce stricter type checking, relying on the default ament_mypy configuration (which wraps strict mypy settings automatically) seems to be passing perfectly right now. Alternatively, we could pass a custom --config file. Please let me know what you think and how I should proceed. Thank you.
There was a problem hiding this comment.
If you update to the newest version on rolling it shouldn't crash.
There was a problem hiding this comment.
Please correct me if I'm wrong but I think we may need to provide argv=['--ament-strict']. If we just pass the raw string into main(), Python's argparse breaks the word apart and tries to read it letter-by-letter, which causes it to crash.
There was a problem hiding this comment.
Sorry yes that is what I meant.
There was a problem hiding this comment.
I just pushed the update using argv=['--ament-strict'] and the letter-by-letter parsing crash seems to be gone but, the CI build is still failing with a new SystemExit: 2 from argparse because --ament-strict is throwing an 'unrecognized argument' error. Looking at the CI traceback, the server is executing ament_mypy from /opt/ros/rolling/..., meaning it's using the pre-built binaries. It looks like the ubuntu_noble CI containers haven't been synced with the latest version on rolling I think which is causing the build to fail.
How do you think we should proceed?
Signed-off-by: Jonathan Setiawan <jonathanrustam2@gmail.com>
|
Pulls: #780 |
Added test_mypy.py file and resolved all regressions.
Fixes #767