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.

Sign up for GitHub

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

Jump to bottom

Correct bool reading from yaml files #415

Merged
merged 2 commits into from
Apr 12, 2019
Merged

Correct bool reading from yaml files #415

merged 2 commits into from
Apr 12, 2019

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Apr 11, 2019 •
edited
Loading

See ros2/launch_ros#12 (comment)

Basically, value="of" was comparing true with 'off', and it shouldn't.
Combining strncmp with strnlen is not a good idea. strcmp is enough, it returns only 0 when the strings are equal and have the same length.

Edit:
I have also added an extra checking. If a value is quoted, it's always detected as a string:

a: 'true'
b: true

Before, both a and b where recognized like bools. Now it is corrected.

@ivanpauno ivanpauno added the in review Waiting for review (Kanban column) label Apr 11, 2019
@ivanpauno ivanpauno self-assigned this Apr 11, 2019
@wjwwood
Copy link
Member

wjwwood commented Apr 11, 2019

You should run CI to look for Windows warnings, as it can be picky about these functions.

@wjwwood
Copy link
Member

wjwwood commented Apr 11, 2019

(you should always run CI, but in case anyone was temped to call this trivial and just merge it)

wjwwood
wjwwood approved these changes Apr 11, 2019
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm

@ivanpauno ivanpauno changed the title Replace some strncmp calls with strcmp Correct bool reading from yaml files Apr 12, 2019
@ivanpauno
Copy link
Member Author

CI:

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

@ivanpauno
Copy link
Member Author

@anup-pem @wjwwood Is this ok for merging?
Here is the documentation about yaml_scalar_style_e.

@wjwwood wjwwood merged commit 11b5eb8 into master Apr 12, 2019
@wjwwood wjwwood deleted the ivanpauno/fix-yaml-parser-bool-comparisons branch April 12, 2019 22:25
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Apr 12, 2019
nuclearsandwich added a commit to ros/rosdistro that referenced this pull request Apr 15, 2019
@nuclearsandwich
rcl: 0.7.0-3 in 'dashing/distribution.yaml' [bloom] (#20938)
70aa5ce 
Increasing version of package(s) in repository `rcl` to `0.7.0-3`:

- upstream repository: https://github.com/ros2/rcl.git
- release repository: https://github.com/ros2-gbp/rcl-release.git
-   file: `dashing/distribution.yaml`
- bloom version: `0.8.0`
- previous version for package: `0.7.0-2`

## rcl

```
* Added more test cases for graph API + fix bug. (#404 <ros2/rcl#404>)
* Fixed missing include. (#413 <ros2/rcl#413>)
* Updated to use pedantic. (#412 <ros2/rcl#412>)
* Added function to get publisher actual qos settings. (#406 <ros2/rcl#406>)
* Refactored graph API docs. (#401 <ros2/rcl#401>)
* Updated to use ament_target_dependencies where possible. (#400 <ros2/rcl#400>)
* Fixed regression around fully qualified node name. (#402 <ros2/rcl#402>)
* Added function rcl_names_and_types_init. (#403 <ros2/rcl#403>)
* Fixed uninitialize sequence number of client. (#395 <ros2/rcl#395>)
* Added launch along with launch_testing as test dependencies. (#393 <ros2/rcl#393>)
* Set symbol visibility to hidden for rcl. (#391 <ros2/rcl#391>)
* Updated to split test_token to avoid compiler note. (#392 <ros2/rcl#392>)
* Dropped legacy launch API usage. (#387 <ros2/rcl#387>)
* Improved security directory lookup. (#332 <ros2/rcl#332>)
* Enforce non-null argv values on rcl_init(). (#388 <ros2/rcl#388>)
* Removed incorrect argument documentation. (#361 <ros2/rcl#361>)
* Changed error to warning for multiple loggers. (#384 <ros2/rcl#384>)
* Added rcl_node_get_fully_qualified_name. (#255 <ros2/rcl#255>)
* Updated rcl_remap_t to use the PIMPL pattern. (#377 <ros2/rcl#377>)
* Fixed documentation typo. (#376 <ros2/rcl#376>)
* Removed test circumvention now that a bug is fixed in rmw_opensplice. (#368 <ros2/rcl#368>)
* Updated to pass context to wait set, and fini rmw context. (#373 <ros2/rcl#373>)
* Updated to publish logs to Rosout. (#350 <ros2/rcl#350>)
* Contributors: AAlon, Dirk Thomas, Jacob Perron, M. M, Michael Carroll, Michel Hidalgo, Mikael Arguedas, Nick Burek, RARvolt, Ross Desmond, Sachin Suresh Bhat, Shane Loretz, William Woodall, ivanpauno
```

## rcl_action

```
* Added Action graph API (#411 <ros2/rcl#411>)
* Updated to use ament_target_dependencies where possible. (#400 <ros2/rcl#400>)
* Fixed typo in Doxyfile. (#398 <ros2/rcl#398>)
* Updated tests to use separated action types. (#340 <ros2/rcl#340>)
* Fixed minor documentation issues. (#397 <ros2/rcl#397>)
* Set symbol visibility to hidden for rcl. (#391 <ros2/rcl#391>)
* Fixed rcl_action documentation. (#380 <ros2/rcl#380>)
* Removed now unused test executable . (#382 <ros2/rcl#382>)
* Removed unused action server option 'clock_type'. (#382 <ros2/rcl#382>)
* Set error message when there is an invalid goal transition. (#382 <ros2/rcl#382>)
* Updated to pass context to wait set, and fini rmw context (#373 <ros2/rcl#373>)
* Contributors: Dirk Thomas, Jacob Perron, Sachin Suresh Bhat, William Woodall, ivanpauno
```

## rcl_lifecycle

```
* Updated to use ament_target_dependencies where possible. (#400 <ros2/rcl#400>)
* Set symbol visibility to hidden for rcl. (#391 <ros2/rcl#391>)
* Contributors: Sachin Suresh Bhat, ivanpauno
```

## rcl_yaml_param_parser

```
* Corrected bool reading from yaml files. (#415 <ros2/rcl#415>)
* Added launch along with launch_testing as test dependencies. (#393 <ros2/rcl#393>)
* Set symbol visibility to hidden for rcl. (#391 <ros2/rcl#391>)
* Contributors: Michel Hidalgo, Sachin Suresh Bhat, ivanpauno
```
andre-rosa pushed a commit to andre-rosa/rosdistro that referenced this pull request May 3, 2019
@nuclearsandwich @andre-rosa
rcl: 0.7.0-3 in 'dashing/distribution.yaml' [bloom] (ros#20938)
4d7dfa8 
Increasing version of package(s) in repository `rcl` to `0.7.0-3`:

- upstream repository: https://github.com/ros2/rcl.git
- release repository: https://github.com/ros2-gbp/rcl-release.git
-   file: `dashing/distribution.yaml`
- bloom version: `0.8.0`
- previous version for package: `0.7.0-2`

## rcl

```
* Added more test cases for graph API + fix bug. (ros#404 <ros2/rcl#404>)
* Fixed missing include. (ros#413 <ros2/rcl#413>)
* Updated to use pedantic. (ros#412 <ros2/rcl#412>)
* Added function to get publisher actual qos settings. (ros#406 <ros2/rcl#406>)
* Refactored graph API docs. (ros#401 <ros2/rcl#401>)
* Updated to use ament_target_dependencies where possible. (ros#400 <ros2/rcl#400>)
* Fixed regression around fully qualified node name. (ros#402 <ros2/rcl#402>)
* Added function rcl_names_and_types_init. (ros#403 <ros2/rcl#403>)
* Fixed uninitialize sequence number of client. (ros#395 <ros2/rcl#395>)
* Added launch along with launch_testing as test dependencies. (ros#393 <ros2/rcl#393>)
* Set symbol visibility to hidden for rcl. (ros#391 <ros2/rcl#391>)
* Updated to split test_token to avoid compiler note. (ros#392 <ros2/rcl#392>)
* Dropped legacy launch API usage. (ros#387 <ros2/rcl#387>)
* Improved security directory lookup. (ros#332 <ros2/rcl#332>)
* Enforce non-null argv values on rcl_init(). (ros#388 <ros2/rcl#388>)
* Removed incorrect argument documentation. (ros#361 <ros2/rcl#361>)
* Changed error to warning for multiple loggers. (ros#384 <ros2/rcl#384>)
* Added rcl_node_get_fully_qualified_name. (ros#255 <ros2/rcl#255>)
* Updated rcl_remap_t to use the PIMPL pattern. (ros#377 <ros2/rcl#377>)
* Fixed documentation typo. (ros#376 <ros2/rcl#376>)
* Removed test circumvention now that a bug is fixed in rmw_opensplice. (ros#368 <ros2/rcl#368>)
* Updated to pass context to wait set, and fini rmw context. (ros#373 <ros2/rcl#373>)
* Updated to publish logs to Rosout. (ros#350 <ros2/rcl#350>)
* Contributors: AAlon, Dirk Thomas, Jacob Perron, M. M, Michael Carroll, Michel Hidalgo, Mikael Arguedas, Nick Burek, RARvolt, Ross Desmond, Sachin Suresh Bhat, Shane Loretz, William Woodall, ivanpauno
```

## rcl_action

```
* Added Action graph API (ros#411 <ros2/rcl#411>)
* Updated to use ament_target_dependencies where possible. (ros#400 <ros2/rcl#400>)
* Fixed typo in Doxyfile. (ros#398 <ros2/rcl#398>)
* Updated tests to use separated action types. (ros#340 <ros2/rcl#340>)
* Fixed minor documentation issues. (ros#397 <ros2/rcl#397>)
* Set symbol visibility to hidden for rcl. (ros#391 <ros2/rcl#391>)
* Fixed rcl_action documentation. (ros#380 <ros2/rcl#380>)
* Removed now unused test executable . (ros#382 <ros2/rcl#382>)
* Removed unused action server option 'clock_type'. (ros#382 <ros2/rcl#382>)
* Set error message when there is an invalid goal transition. (ros#382 <ros2/rcl#382>)
* Updated to pass context to wait set, and fini rmw context (ros#373 <ros2/rcl#373>)
* Contributors: Dirk Thomas, Jacob Perron, Sachin Suresh Bhat, William Woodall, ivanpauno
```

## rcl_lifecycle

```
* Updated to use ament_target_dependencies where possible. (ros#400 <ros2/rcl#400>)
* Set symbol visibility to hidden for rcl. (ros#391 <ros2/rcl#391>)
* Contributors: Sachin Suresh Bhat, ivanpauno
```

## rcl_yaml_param_parser

```
* Corrected bool reading from yaml files. (ros#415 <ros2/rcl#415>)
* Added launch along with launch_testing as test dependencies. (ros#393 <ros2/rcl#393>)
* Set symbol visibility to hidden for rcl. (ros#391 <ros2/rcl#391>)
* Contributors: Michel Hidalgo, Sachin Suresh Bhat, ivanpauno
```
@ivanpauno ivanpauno mentioned this pull request Jun 21, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood approved these changes

Assignees

@ivanpauno ivanpauno

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ivanpauno @wjwwood