Skip to content

[rcl_interfaces] Homogenize iface definition#23

Merged
mikaelarguedas merged 4 commits intomasterfrom
homogenize_iface_definition
Nov 3, 2017
Merged

[rcl_interfaces] Homogenize iface definition#23
mikaelarguedas merged 4 commits intomasterfrom
homogenize_iface_definition

Conversation

@mikaelarguedas
Copy link
Copy Markdown
Member

This is just cleanup

  • first commit make msg/srv definition consistent 27920fa
  • second commit moves the repo's README.md to the right folder 89da210
  • thiird commit creates a generic README for the entire repo 5cd70c2

README.md Outdated
* List the parameters on this node matching the filters.
* `set_parameters`: `SetParameters`
* Set parameters on this node.
A set of packages which contain interface files (.msg and .srv) used for the internal implementation and testing of the client libraries.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wouldn't consider those "internal". Most of them define public interfaces, no?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

"used for the implementation and testing" then ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"defining the interface across client libraries"?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmm not sure because as you said it is user facing and currently most of them are not used to communicate across client libraries given that they're used mostly/(only?) in rclcpp.

@ros2/team maybe some native speakers have some insights ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

They're definitely public and I would say their primary use is implementation of rcl concepts rather than testing, though they could be used for that too I guess.

"This repository contains a set of packages which themselves contain interface files (i.e. .msg and .srv files) used to implement client library concepts like parameters."

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

my goal here is to give a description of the content of the repository and not only describe the rcl_interfaces package, that's why I talked about testing (given that this repo also contains test_msgs).

I'm fine with "This repository contains a set of packages which themselves contain interface files (i.e. .msg and .srv files) used to implement client library concepts" I just wanted to clarify the scope

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, I was confused by the diff and I didn't know there were test messages here. In that case I agree, something like:

"This repository contains a set of packages which themselves contain interface files (i.e. .msg and .srv files) which are used both to implement client library concepts and in testing."

Or something like that.

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've updated it with approximately the verbiage of above.

@dhood dhood added the in review Waiting for review (Kanban column) label Nov 2, 2017
@mikaelarguedas
Copy link
Copy Markdown
Member Author

Thanks @tfoote
@ros2/team reay for review

@mikaelarguedas mikaelarguedas merged commit 7710246 into master Nov 3, 2017
@mikaelarguedas mikaelarguedas deleted the homogenize_iface_definition branch November 3, 2017 21:12
@mikaelarguedas mikaelarguedas removed the in review Waiting for review (Kanban column) label Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants