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

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

Merged
merged 7 commits into from Apr 23, 2019
Merged

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

merged 7 commits into from Apr 23, 2019

Conversation

mbuijs
Copy link
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
Member

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

@mbuijs
Copy link
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
Member

Would you prefer this also to be updated in tests?

Yes, that would be good.

@mbuijs
Copy link
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
Contributor Author

mbuijs commented Apr 4, 2019 •
edited

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
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
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

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
tools/roslaunch/src/roslaunch/loader.py Outdated Show resolved Hide resolved
tools/rosparam/src/rosparam/__init__.py Outdated Show resolved Hide resolved
tools/rosparam/src/rosparam/__init__.py Outdated Show resolved Hide resolved
@mbuijs
Copy link
Contributor Author

mbuijs commented Apr 15, 2019

@ros-pull-request-builder retest this please

@mbuijs
Copy link
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
Copy link
Member

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
Open
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 join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

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