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.

Sign up for GitHub

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

Jump to bottom

Encourage users to always provide a QoS history depth #344

Merged
merged 7 commits into from
May 14, 2019
Merged

Encourage users to always provide a QoS history depth #344

merged 7 commits into from
May 14, 2019

Conversation

jacobperron
Copy link
Member

Resolves #342

  • Added a new parameter to methods for creating publishers and subscriptions

    The new parameter is required and must be a QoSProfile object or an integer value for the history depth.
    For now, it is possible to not use the parameter for backwards compatibility, but a deprecation warning is made.

  • Add deprecation warning when constructing a QoSProfile without a history depth

    If the history setting is KEEP_ALL, then it is not required to pass the depth setting.

@jacobperron jacobperron added the in progress Actively being worked on (Kanban column) label May 10, 2019
@jacobperron jacobperron requested a review from wjwwood May 10, 2019 23:26
@jacobperron
Copy link
Member Author

@wjwwood I'm not sure if this exactly what we discussed. PTAL before I continue with services and actions.

@jacobperron jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 10, 2019
wjwwood
wjwwood reviewed May 11, 2019
View reviewed changes
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.

I think we need a DeprecatedQoSProfile class that inherits from QoSProfile and produces a warning if used with create_*(), and then change the type of qos_profile_default to be that type, but leave the other builtin profiles as the QoSProfile type, so they can be used without warnings.

rclpy/rclpy/qos.py Show resolved Hide resolved
rclpy/test/test_node.py Outdated Show resolved Hide resolved
@jacobperron jacobperron added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels May 13, 2019
@jacobperron
Copy link
Member Author

@wjwwood Thanks for the review!

I've address your comments and will continue to make similar changes for services and actions.

I've refactored the RMW to rclpy conversion function (ea2de90) so it constructs a QoSProfile with keyword arguments. This avoids deprecation warnings when importing rclpy.qos due to the construction of the default profiles.

@jacobperron
Encourage users to always provide a QoS history depth
ea91111 
* Added a new parameter to methods for creating publishers and subscriptions

  The new parameter is required and must be a QoSProfile object or an integer value for the history depth.
  For now, it is possible to not use the parameter for backwards compatibility, but a deprecation warning is made.

* Add deprecation warning when constructing a QoSProfile without a history depth

  If the history setting is KEEP_ALL, then it is not required to pass the depth setting.

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Allow creating QoSProfile with just a depth
a96d03a 
Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Deprecated 'qos_profile_default'
71df77d 
Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Construct QoSProfile with keyword arguments in C extension conversion…
6ad06db 
… function

This is to avoid deprecation warnings.

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Use default UserWarning for visibilty
2f2027d 
DeprecatedWarning is not visible by default until Python 3.7.

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron jacobperron force-pushed the jacob/require_history_depth branch from 1b48d55 to 4afa822 Compare May 13, 2019 23:46
@jacobperron
Fix deprecation warnings
394e93e 
Signed-off-by: Jacob Perron <[email protected]>
@jacobperron jacobperron force-pushed the jacob/require_history_depth branch from 4afa822 to 394e93e Compare May 13, 2019 23:49
@jacobperron
Copy link
Member Author

Rebased on master; ready for review.

@jacobperron jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 13, 2019
@jacobperron
Copy link
Member Author

I forgot to find/replace usage of qos_profile_default with QoSProfile(depth=10). Doing that now.

@jacobperron
Copy link
Member Author

jacobperron commented May 14, 2019 •
edited
Loading

@jacobperron jacobperron merged commit 57ca835 into master May 14, 2019
@delete-merged-branch delete-merged-branch bot deleted the jacob/require_history_depth branch May 14, 2019 16:29
@jacobperron jacobperron removed the in review Waiting for review (Kanban column) label May 14, 2019
@jacobperron jacobperron mentioned this pull request May 21, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Encourage users to always specify QoS history depth
2 participants
@jacobperron @wjwwood