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

Clarify ROS default values will be used in default constructor #184

Merged
merged 4 commits into from
Aug 15, 2018

Conversation

dhood
Copy link
Member

@dhood dhood commented Aug 10, 2018

@dhood dhood added the in review Waiting for review (Kanban column) label Aug 10, 2018
@dhood dhood self-assigned this Aug 10, 2018
@mikaelarguedas
Copy link
Member

I think there is still misunderstanding here.

In this article, "default value" always refers to the default value provided in the msg file.

We don't have the concept of "No default value at all" as we define default values for all the types we define in the ROS IDL format (detailed in this article)

@dhood
Copy link
Member Author

dhood commented Aug 10, 2018

since char wasn't listed in the article with common defaults I thought it was an example of the 'no default' category. I tried to clarify there's always a default in 93a6c39

@mikaelarguedas
Copy link
Member

As this paragraph was duplicated below, is it possible to update the following lines as well ?

@dhood
Copy link
Member Author

dhood commented Aug 15, 2018

Other sentence updated in 77de52d

@mikaelarguedas
Copy link
Member

lgtm.
Maybe @clalancette wants to review as he has the most context on this

@clalancette
Copy link
Contributor

This clarification looks fine to me! I just corrected some spelling, otherwise, I think this is good to go.

@dhood dhood merged commit 131ce29 into gh-pages Aug 15, 2018
@dhood dhood deleted the clarify_default_zeros branch August 15, 2018 17:49
@dhood dhood removed the in review Waiting for review (Kanban column) label Aug 15, 2018
@dhood
Copy link
Member Author

dhood commented Aug 15, 2018

thanks to you both!

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.

3 participants