Skip to content

Conversation

@ivanpauno
Copy link
Collaborator

@ivanpauno ivanpauno commented Aug 21, 2020

@ivanpauno ivanpauno added the enhancement New feature or request label Aug 21, 2020
@ivanpauno ivanpauno self-assigned this Aug 21, 2020
@ivanpauno ivanpauno changed the title Add OfferedDeadlineMissing QoS event Add OfferedDeadlineMissed QoS event Aug 21, 2020
Base automatically changed from ivanpauno/reorganize-statuses-package to feature/events August 24, 2020 16:33
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno force-pushed the ivanpauno/offered-deadline-missed branch from cfeb741 to 9dc7656 Compare August 24, 2020 16:39
@ivanpauno ivanpauno marked this pull request as ready for review August 24, 2020 16:39
@ivanpauno ivanpauno requested a review from jacobperron August 24, 2020 16:39
Comment on lines +29 to +30
public int totalCount;
public int totalCountChange;

Choose a reason for hiding this comment

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

I missed this for other statuses in previous PRs, but I think these member variables should be private and we can add getters for them. I think this will provide a bit better encapsulation (e.g. if symbol names change in the future, it's easier to deprecate a method than a member variable).

I don't think making them private has any impact on the native code setting their values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that passive data types only carrying data are a valid citizen in oop.
When the class doesn't have any invariant (relationship between fields) there's not much value on using private data, setters and getters.

if symbol names change in the future, it's easier to deprecate a method than a member variable

If we change the name of one of the members we're going to break all other clients, so I don't think that's going to happen.


I realized (late), that the generated java bindings for ROS messages doesn't expose the data and provides getters/setters instead.
Though I don't like that, maybe being consistent in event statuses is better.

If would prefer doing this change after merging all remaining PRs in the feature branch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Merged, we can solve this issue before merging the feature branch on master.

Choose a reason for hiding this comment

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

I'm ambivalent about it. Maybe we just leave it as-is and bring it up again when we open a PR upstream.

I'm not sure what the reasoning was to generating getters and setters for ROS messages; this too may be something we can change later upstream.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno merged commit 3300418 into feature/events Aug 26, 2020
@ivanpauno ivanpauno deleted the ivanpauno/offered-deadline-missed branch August 26, 2020 14:34
ivanpauno added a commit that referenced this pull request Aug 31, 2020
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
ivanpauno added a commit that referenced this pull request May 17, 2021
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
ivanpauno added a commit to ros2-java/ros2_java that referenced this pull request Jan 14, 2022
ivanpauno added a commit to ros2-java/ros2_java that referenced this pull request Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants