Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

Switch to yaml.safe_load(_all) to prevent YAMLLoadWarning#1688

Merged
dirk-thomas merged 7 commits intoros:melodic-develfrom
mbuijs:fix-yaml-load-warning
Apr 23, 2019
Merged

Switch to yaml.safe_load(_all) to prevent YAMLLoadWarning#1688
dirk-thomas merged 7 commits intoros:melodic-develfrom
mbuijs:fix-yaml-load-warning

Conversation

@mbuijs
Copy link
Copy Markdown
Contributor

@mbuijs mbuijs commented Apr 3, 2019

Fixes #1686.

In yaml/pyyaml#265 it's stated that safe_load always existed for this reason, I did a quick double check and it traces back at least to 2006, so this might even be safe to backport to older releases.

@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Apr 3, 2019

Thanks for the patch. Can you maybe also update the other places in this repo where these functions are used?

@mbuijs
Copy link
Copy Markdown
Contributor Author

mbuijs commented Apr 3, 2019

I can have a look. Would you prefer this also to be updated in tests?

@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Apr 3, 2019

Would you prefer this also to be updated in tests?

Yes, that would be good.

@mbuijs
Copy link
Copy Markdown
Contributor Author

mbuijs commented Apr 4, 2019

So I had not realized about the following (from https://wiki.ros.org/rosparam):

There are also special converters for angle radian/degree representations. Any Python-legal mathematical expression can be used with the radian value, with pi used to represent pi.

I'm doing some more changes to support this functionality while using the SafeLoader provided by pyyaml.

Furthermore I'll also try to see if there are other places where more features than the SafeLoader provides, so far I had just done a rapid search and replace...

@mbuijs
Copy link
Copy Markdown
Contributor Author

mbuijs commented Apr 4, 2019 •
edited
Loading

Additional question: which other tools aside of rosparam and roslaunch should have the ability to do the degrees and radians conversion functionality when evaluating YAML strings?

Edit: never mind, I see that also currently it is only supported in these 2, so I'll stick to that.

mbuijs added 2 commits April 4, 2019 18:21
@mbuijs
Extend PyYAML's SafeLoader and use it with yaml.load
a7dc957 
Also added convenience functions for using this loader for reuse in
`roslaunch`
@mbuijs
fix typo in rosparam.yaml_load_all
68d778d
@mbuijs
Copy link
Copy Markdown
Contributor Author

mbuijs commented Apr 5, 2019

So tests are passing now.
My experience with python is limited, I'm open to feedback, especially on how I applied the change in rosparam and roslaunch

  1. attempting to not change the SafeLoader instance in PyYAML
  2. adding the convencience functions yaml_load and yaml_load_all inside rosparam.

@marting87
Copy link
Copy Markdown

marting87 commented Apr 11, 2019

Changes look to me, since I'm getting a lot of YAMLLoadWarning in my console I hope to see this merged.

dirk-thomas
dirk-thomas reviewed Apr 11, 2019
View reviewed changes
@mbuijs
Copy link
Copy Markdown
Contributor Author

mbuijs commented Apr 15, 2019

@ros-pull-request-builder retest this please

@mbuijs
Copy link
Copy Markdown
Contributor Author

mbuijs commented Apr 19, 2019

@dirk-thomas I processed your remarks and all checks have passed. Would you mind doing another check and merging if it's good?

dirk-thomas
dirk-thomas reviewed Apr 22, 2019
View reviewed changes
@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Apr 23, 2019

Thank you for iterating on this.

@dirk-thomas dirk-thomas merged commit 29053c4 into ros:melodic-devel Apr 23, 2019
@cwecht cwecht mentioned this pull request Aug 11, 2019
Closed
130s pushed a commit to 130s/ros_comm that referenced this pull request Nov 28, 2019
@mbuijs @130s
Switch to yaml.safe_load(_all) to prevent YAMLLoadWarning (ros#1688)
5046833 
* Switch to yaml.safe_load(_all) to prevent YAMLLoadWarning

* Change all usages of yaml.load to yaml.safe_load

* Extend PyYAML's SafeLoader and use it with `yaml.load`

Also added convenience functions for using this loader for reuse in
`roslaunch`

* fix typo in rosparam.yaml_load_all

* Modify Loader and SafeLoader in yaml module directly

* Revert whitespace change

* Revert unrelated change to import through global variable construction
@130s 130s mentioned this pull request Nov 28, 2019
Closed
@mbuijs mbuijs deleted the fix-yaml-load-warning branch June 9, 2020 09:33
@dirk-thomas dirk-thomas mentioned this pull request Aug 4, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@dirk-thomas dirk-thomas dirk-thomas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

consider-backport enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

rosparam YAMLLoadWarning

3 participants

@mbuijs @dirk-thomas @marting87