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

Provide support for yaml constructors !degrees and !radians #252

Merged
merged 2 commits into from
Aug 18, 2020

Conversation

rhaschke
Copy link
Contributor

Addresses #251. @gavanderhoorn, please review.

@gavanderhoorn
Copy link
Contributor

This is 💯 again @rhaschke.

My intention with #251 was not to come up with more work for you, but I'm of course very happy you decided to implement it.

I'll run some tests and let you know.

Copy link
Contributor

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

This works for me 👍

thanks for adding this @rhaschke.

@gavanderhoorn
Copy link
Contributor

Do I understand the code correctly when I say !radians appears to support "arbitrary expressions", while !degrees doesn't?

@rhaschke
Copy link
Contributor Author

Do I understand the code correctly when I say !radians appears to support "arbitrary expressions", while !degrees doesn't?

Yes, that's what I copied from ros_param. Do you want to change this?

@gavanderhoorn
Copy link
Contributor

Yes, that's what I copied from ros_param. Do you want to change this?

We should probably keep the same restrictions as in rosparam, so I would vote to keep it as-is.

Only thing to make sure perhaps is the eval(..) there: does it open up xacro to security issues? Should we limit / sanatise something perhaps?

@rhaschke
Copy link
Contributor Author

Indeed, we should restrict eval() to our limited set of functions...

@gavanderhoorn
Copy link
Contributor

@rhaschke: just submitted ubi-agni#4, which adds some (primitive) testing to the safe(r) eval(..) functionality when used in the yaml ctors.

@gavanderhoorn
Copy link
Contributor

Thanks @rhaschke 👍

@gavanderhoorn
Copy link
Contributor

@rhaschke: would you have time to release a new version of xacro which includes this change?

@rhaschke
Copy link
Contributor Author

Done: ros/rosdistro#26352

@gavanderhoorn
Copy link
Contributor

@rhaschke: I'm not sure I remember your syncing/back-porting policy between branches here.

Would it be possible to get this in Noetic as well?

@rhaschke
Copy link
Contributor Author

Sure. I will prepare a new release later this evening.

@rhaschke
Copy link
Contributor Author

Done: ros/rosdistro#26945

@gavanderhoorn
Copy link
Contributor

Thanks @rhaschke 👍

@Briancbn
Copy link

Will this be merged into foxy release? I have to merge this to get the melodic-devel-staging urdf models working in ROS2.

@rhaschke
Copy link
Contributor Author

@Briancbn: Sure. Do you mind preparing a merge PR? This would help to speed up the process.

@Briancbn
Copy link

Briancbn commented Nov 9, 2020

@rhaschke could you help review the PR? thanks.

@rhaschke
Copy link
Contributor Author

rhaschke commented Nov 9, 2020

#259 is on my todo list. But there is just too much other work...

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