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

Added dockblock to ComponentManager class #1102

Merged
merged 2 commits into from
May 13, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented May 4, 2020

Added dockblock to ComponentManager class

Signed-off-by: ahcorde ahcorde@gmail.com

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde added enhancement New feature or request documentation labels May 4, 2020
@ahcorde ahcorde self-assigned this May 4, 2020
@ahcorde ahcorde requested a review from brawner May 11, 2020 09:24
RCLCPP_COMPONENTS_PUBLIC
virtual std::shared_ptr<rclcpp_components::NodeFactory>
create_component_factory(const ComponentResource & resource);

protected:
// Service to load a new node in the component
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the service callback, right? Say that instead. Also, when might a deriving class want to override these functions?

These questions apply also to the next two functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you derive from this class and change these methods we are going to change the behaviour of this abstraction. do you think we should explicitly indicate it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just surprised to see protected virtual callbacks, and I wasn't entirely sure when that would be useful to override, so I thought some documentation might help. But seeing as all the public functions are virtual as well, I guess this class was just designed with that sort of flexibility in mind.

Signed-off-by: ahcorde <ahcorde@gmail.com>
RCLCPP_COMPONENTS_PUBLIC
virtual std::shared_ptr<rclcpp_components::NodeFactory>
create_component_factory(const ComponentResource & resource);

protected:
// Service to load a new node in the component
Copy link
Contributor

Choose a reason for hiding this comment

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

I was just surprised to see protected virtual callbacks, and I wasn't entirely sure when that would be useful to override, so I thought some documentation might help. But seeing as all the public functions are virtual as well, I guess this class was just designed with that sort of flexibility in mind.

@ahcorde
Copy link
Contributor Author

ahcorde commented May 12, 2020

running CI up-to rclcpp_components to check linters

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

@ahcorde ahcorde merged commit ce4c873 into master May 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the ahcorde/docblock/rclcpp_components branch May 13, 2020 06:18
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Jul 7, 2020
* Added dockblock to ComponentManager class

Signed-off-by: ahcorde <ahcorde@gmail.com>

* added feedback

Signed-off-by: ahcorde <ahcorde@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants