Skip to content

Added dockblock to ComponentManager class#1102

Merged
ahcorde merged 2 commits intomasterfrom
ahcorde/docblock/rclcpp_components
May 13, 2020
Merged

Added dockblock to ComponentManager class#1102
ahcorde merged 2 commits intomasterfrom
ahcorde/docblock/rclcpp_components

Conversation

@ahcorde
Copy link
Copy Markdown
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
create_component_factory(const ComponentResource & resource);

protected:
// Service to load a new node in the component
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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>
create_component_factory(const ComponentResource & resource);

protected:
// Service to load a new node in the component
Copy link
Copy Markdown
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
Copy Markdown
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.

3 participants