Skip to content

Conversation

@mbarkhau
Copy link
Contributor

Fixes #95
Don't evaluate top level ast.BinOp arguments, treat them as strings instead.

@googlebot
Copy link
Collaborator

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@dbieber
Copy link
Collaborator

dbieber commented Oct 11, 2017

Thanks for submitting this.
Can you confirm it doesn't break parsing negative numbers?

@mbarkhau
Copy link
Contributor Author

I added an assertion, yes looks like negative numbers are still parsed as before to type int or float. I guess the expression must be ast.UnaryOp or something.

fire/parser.py Outdated
"""
root = ast.parse(value, mode='eval')
if isinstance(root.body, ast.BinOp):
return value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's raise a ValueError here instead, so that the logic of "treat this thing that doesn't parse as a string" all exists in one place (line 74).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh right, the fallback logic is to treat it as a string anyway. Good point.

@dbieber
Copy link
Collaborator

dbieber commented Oct 11, 2017

Thanks for adding the negative number tests in

@dbieber dbieber merged commit 7400ca8 into google:master Oct 11, 2017
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