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

python builtins: allow max, min #178

Closed
wants to merge 1 commit into from
Closed

Conversation

scpeters
Copy link
Contributor

@scpeters scpeters commented Feb 8, 2018

Only a subset of python builtin functions are permitted in xacro ${} expressions for security reasons, for reasons listed here. I recently wanted to use min and max in a xacro file, so I wonder if they can be added to this list.

I looked at the list of python builtin functions that could be useful for math operations, and many of them have alternatives since we import math.*:

  • abs: use math.fabs
  • len: use .__len__() object method
  • pow: use **
  • sum: use math.fsum

The only other one I would consider adding is round. You can get partial functionality with math.floor and math.ceil, but not the same. Currently, though, I just need max and min. I would propose back porting this to kinetic if it is acceptable.

cc @sloretz @clalancette

@codebot
Copy link
Contributor

codebot commented Feb 8, 2018

This sounds reasonable. Let's create a melodic branch and start putting stuff there. I'd also like to finally get the unicode PR merged in for melodic.

@scpeters
Copy link
Contributor Author

scpeters commented Feb 9, 2018

melodic sounds reasonable to me; though I'm not boss enough to create a melodic branch on this repo

@codebot
Copy link
Contributor

codebot commented Feb 21, 2018

I just created a melodic-devel branch.

@scpeters
Copy link
Contributor Author

#179 made for melodic

@rhaschke
Copy link
Contributor

Manually merged due to conflicts.

@rhaschke rhaschke closed this Mar 27, 2018
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