Skip to content

a method to dump all elements of a parameter as yaml#91

Merged
tfoote merged 1 commit intomasterfrom
parameter_to_yaml
Oct 20, 2015
Merged

a method to dump all elements of a parameter as yaml#91
tfoote merged 1 commit intomasterfrom
parameter_to_yaml

Conversation

@tfoote
Copy link
Copy Markdown
Contributor

@tfoote tfoote commented Aug 17, 2015

This is a helper function I wrote when debugging the parameters I found it very useful to be able to print all 3 pieces of info to the screen quickly. I think it would be helpful to have similar functionality. Maybe a single line json style encoding would be more reuseable than this current one with newlines.

I know it's an expansion of the API but I think this will save people time down the line.

The current one isn't polished but I wanted people's feedback.

Thoughts?

@tfoote tfoote added the in progress Actively being worked on (Kanban column) label Aug 17, 2015
@esteve
Copy link
Copy Markdown
Member

esteve commented Aug 17, 2015

+1, very useful!

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Aug 17, 2015

Out of curiosity why the flat structure and not the key-value structure like this:

<name>:
    type: <type>
    value: <value>

@tfoote
Copy link
Copy Markdown
Contributor Author

tfoote commented Aug 17, 2015

That's probably a better structure. It started as a cout and became 3, then migrated to yaml like syntax.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Aug 17, 2015

It's not clear to me that one way is better than the other, since one is useful in dicts, but the other is useful as an element in a list. I'm just trying to imagine how you expect this to be used by users, by the system, or to recursively to build a tree of parameters.

@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Aug 19, 2015

Instead of providing this functionality through a member function I would suggest to provide it either in a free function or through an yaml-specific entity. May be then also variations of this can be provided to match the different use cases @wjwwood mentioned.

@tfoote tfoote force-pushed the parameter_to_yaml branch 3 times, most recently from da9fd71 to 9c98522 Compare August 20, 2015 23:16
@tfoote
Copy link
Copy Markdown
Contributor Author

tfoote commented Aug 20, 2015

Updated to be free functions and support both list and dict entry elements as well as turning a vector of params into a dict.

There are unit tests added to ros2/system_tests#14 as well.

It's ready for review but not necessary for the demos, so I'll hold off on triggering tests and it can be reviewed later.

@tfoote tfoote added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 20, 2015
@esteve
Copy link
Copy Markdown
Member

esteve commented Aug 20, 2015

+1 again

@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Aug 20, 2015

The functions can't be used to built nested output as-is. I think they should allow to use variable indentation to support that use case.

Depending on what the future might bring (xml output, string output, yaml output for other classes) it might be better to move these cross cutting aspects into a separate file.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Aug 20, 2015

lgtm, is this being used somewhere? I'd feel better about these functions being added if we had an actual example of them being used.

@tfoote
Copy link
Copy Markdown
Contributor Author

tfoote commented Aug 21, 2015

@dirk-thomas There is no indentation they are one line json elements. Why can't they be nested? Each output is a valid block of json. It can be embedded in a list or dropped as a value in a dict. Except the intermediate helper function which produces a dict entry for use in building a multi parameter dict as used in the vector section.

Separating them to another file makes sense. I'll do that.

@wjwwood I used them for printf debugging, they got removed once I finished debugging. This is mostly a convenient way to print all three values quickly.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Aug 21, 2015

Well, if it's just for debugging and printing, then a stream operator and or specialization of std::to_string would be appropriate. The fact that the output is json or yaml compatible sort of doesn't matter.

@tfoote tfoote added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Aug 21, 2015
tfoote added a commit to ros2/system_tests that referenced this pull request Aug 28, 2015
@tfoote @esteve
add tests for to_json ros2/rclcpp#91
fa86328
@esteve esteve mentioned this pull request Aug 28, 2015
Merged
@tfoote
Copy link
Copy Markdown
Contributor Author

tfoote commented Sep 28, 2015

Refactored with @wjwwood 's suggestions.

It's failing due to https://github.com/bengardner/uncrustify/issues/379

CI:

I will work around the above error and squash.

This was referenced Sep 28, 2015
Merged
Merged
@tfoote
Copy link
Copy Markdown
Contributor Author

tfoote commented Sep 28, 2015

New CI:

@tfoote tfoote added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 28, 2015
@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Sep 28, 2015

lgtm.

I'm not crazy about the _ prefixed function, but it should be fine.

@wjwwood wjwwood added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Oct 7, 2015
@tfoote
Copy link
Copy Markdown
Contributor Author

tfoote commented Oct 14, 2015

Comments resolved new CI passing:

@tfoote tfoote added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 14, 2015
dirk-thomas
dirk-thomas reviewed Oct 14, 2015
View reviewed changes
rclcpp/include/rclcpp/parameter.hpp
Copy link
Copy Markdown
Member

@dirk-thomas dirk-thomas Oct 14, 2015

Choose a reason for hiding this comment

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

This method seems to only operate on the ParameterValue. Why is it provided in the ParameterVariant class?

Copy link
Copy Markdown
Contributor Author

@tfoote tfoote Oct 20, 2015

Choose a reason for hiding this comment

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

This provides the string for the value of a ParameterVariant. The ParameterValue is a private member of the ParameterVariant an inaccessible to the user. The method could be moved into ParameterValue except that it's a message. It could be a free function. But then we'd want to have a method on the ParameterVariant to expose it. We can refactor it to use a free function, but I just renamed it to disambiguate it from the new functions which print all the info of the ParameterVariant to string not just the value.

Copy link
Copy Markdown
Contributor Author

@tfoote tfoote Oct 20, 2015

Choose a reason for hiding this comment

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

From the meeting @dirk-thomas will make a more general ticket about this design issue for us to have a standard approach.

tfoote added a commit that referenced this pull request Oct 20, 2015
@tfoote
Merge pull request #91 from ros2/parameter_to_yaml
dba12cb 
a method to dump all elements of a parameter as yaml
@tfoote tfoote merged commit dba12cb into master Oct 20, 2015
@tfoote tfoote removed the in review Waiting for review (Kanban column) label Oct 20, 2015
@tfoote tfoote deleted the parameter_to_yaml branch October 20, 2015 23:01
@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Oct 21, 2015

I have created ros2/ros2#130 to track that.

@dirk-thomas dirk-thomas mentioned this pull request Sep 5, 2019
Open
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
@Karsten1987
disable cppcheck (ros2#91)
4964757
mauropasse pushed a commit to mauropasse/rclcpp that referenced this pull request Sep 21, 2022
@alsora
Merge pull request ros2#91 from mauropasse/mauro/irobot-humble-updates
f5dfe64 
Restore clear callback on QOSEventHandler destruction
jefferyyjhsu added a commit to jefferyyjhsu/rclcpp that referenced this pull request Oct 5, 2022
@jefferyyjhsu
Revert "Merge pull request ros2#91 from mauropasse/mauro/irobot-humbl…
5e16c3c 
…e-updates"

This reverts commit f5dfe64, reversing
changes made to 8d009a9.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@tfoote tfoote

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tfoote @esteve @wjwwood @dirk-thomas