Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add warnings for reserved Python keywords in interface members #96

Merged
merged 3 commits into from
Feb 11, 2020

Conversation

samiamlabs
Copy link
Contributor

This is a temporary fix related to issue #95. Until a language-specific name mangling is implemented to avoid keywords as described in ros2/design#172, a warning would be nice.

I can add stderr prints for services and actions also if this pull request seems like a good idea.

Example build output from colcon:

Starting >>> dredgebot_msgs
Starting >>> rosidl_generator_py
Finished <<< rosidl_generator_py [7.05s]                                                               
[Processing: dredgebot_msgs]                             
--- stderr: dredgebot_msgs                                  
Member name "from" in the message "LatLonLineSegment" is a reserved keyword in Python and is not supported at the moment. Please use a different name.
---
Finished <<< dredgebot_msgs [39.4s]

Summary: 2 packages finished [39.5s]
  1 package had stderr output: dredgebot_msgs

@wjwwood
Copy link
Member

wjwwood commented Jan 2, 2020

@samiamlabs is this still a WIP?

@samiamlabs
Copy link
Contributor Author

I have not added error-prints for services or actions yet. If you think this PR makes sense I can fix that before merging.

Alternatively, we could just merge this PR and I can make a new one for actions and services.

@wjwwood
Copy link
Member

wjwwood commented Jan 2, 2020

I'd advise you to fix it up to check in actions and services too before removing the WIP, but that's up to you. I do think this pr makes sense in general. @dirk-thomas do you think so too? My one concern is that this will warn on messages that already exist and cannot easily be changed, but maybe we already fixed that. I don't remember.

@wjwwood
Copy link
Member

wjwwood commented Jan 16, 2020

ping @dirk-thomas

Also, @samiamlabs do you think you'll have time to follow up on this? No pressure but it would help us triaging things to know.

@ivanpauno ivanpauno added enhancement New feature or request in review Waiting for review (Kanban column) labels Jan 16, 2020
@wjwwood wjwwood removed their assignment Jan 16, 2020
@samiamlabs
Copy link
Contributor Author

I will try to find some time to work on this at the start of next week :)

@samiamlabs samiamlabs changed the title [WIP] Add warnings for reserved Python keywords in interface members Add warnings for reserved Python keywords in interface members Jan 22, 2020
@samiamlabs
Copy link
Contributor Author

Finally got around to do some more work on this.
Here is an example build output:

[Processing: rosidl_python_test_interfaces]                              
--- stderr: rosidl_python_test_interfaces                                  
Member name 'from' in the message 'ReservedKeywords' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
Member name 'yield' in the message 'ReservedKeywords' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
Member name 'is' in the service request 'ReservedKeywordsRequest' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
Member name 'lambda' in the service response 'ReservedKeywordsResponse' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
Member name 'nonlocal' in the action feedback 'ReservedKeywordsFeedback' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
Member name 'global' in the action goal 'ReservedKeywordsGoal' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
Member name 'pass' in the action result 'ReservedKeywordsResult' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
---
Finished <<< rosidl_python_test_interfaces [30.7s]

I also set up a VSCode "Dev Container" for the nightly ROS2 docker image in case anyone is interested: https://github.com/samiamlabs/ros2-nightly-vscode/tree/reserved-keyword-warnings

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, I'll run CI.

@wjwwood
Copy link
Member

wjwwood commented Jan 24, 2020

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

@wjwwood friendly ping. Is this ready? Does something have to be addressed?

@wjwwood
Copy link
Member

wjwwood commented Jan 30, 2020

There are unresolved linter failures, see: #96 (comment)

@samiamlabs
Copy link
Contributor Author

I pushed a fix for the linter issues a while back @wjwwood
Would be great if you can run CI again to make sure the errors are gone :)

@ivanpauno
Copy link
Member

CI (only fastrtps):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@samiamlabs
Copy link
Contributor Author

We noticed that using "from" in a message caused a runtime syntax error for python when importing the message.
Some of the reserved keywords can probably be accessed using getattr without issues like this.
See #95

@ivanpauno
Copy link
Member

We noticed that using "from" in a message caused a runtime syntax error for python when importing the message.
Some of the reserved keywords can probably be accessed using getattr without issues like this.
See #95

IMO, that could be fixed if we use everywhere in the generated files getattr instead of dot syntax.
If the current generated files raise an error when importing, we should rather fail than log a warning (maybe it's a bad idea, because it can generate problems in existing messages). The other option is to fix the places that makes the imported message to fail (using getattr), and just log a warning indicating the usage of a python keyword.

@ivanpauno
Copy link
Member

@samiamlabs Can you rebase? Most of the test failures are resolved in master.

Signed-off-by: Samuel <samuel@dynorobotics.se>
Signed-off-by: Samuel Lindgren <samuel@dynorobotics.se>
Signed-off-by: Samuel Lindgren <samuel@dynorobotics.se>
@samiamlabs
Copy link
Contributor Author

I pushed a rebase with upstream master just now. Hope that helps with CI :)

@ivanpauno
Copy link
Member

I pushed a rebase with upstream master just now. Hope that helps with CI :)

Thanks! Another round of CI:

  • Linux Build Status

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, @ivanpauno should we merge this or run CI again (since the interactive markers issues have been fixed)?

@ivanpauno
Copy link
Member

CI failure seems unrelated, merging!

@ivanpauno ivanpauno merged commit f19748a into ros2:master Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants