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 3 compatibility #163

Merged
merged 9 commits into from
Sep 15, 2017
Merged

python 3 compatibility #163

merged 9 commits into from
Sep 15, 2017

Conversation

rhaschke
Copy link
Contributor

This is a replacement for #158, fixing (most) remaining issues for python 3.

kartikmohta and others added 8 commits September 15, 2017 08:42
Note that two tests fail due to internal changes in Python 3:
- test_iterable_literals_eval: The order of keys in dict changed in Py3
- test_no_evaluation: ast.literal_eval('5 -2') gives 3 in Py3
avoid calling lex.peek() multiple times
Parsing of number and boolean literals was done with ast.literal_eval().
However, since python3 this function evaluates expressions like "5 -3"
as the number result and not a string anymore.
Hence resort to explicit type-casting to int, float, or boolean.
- handle numeric values at arbitrary locations, e.g. "3.1415 0 1"
- ignore dict order, correctly parsing dict expressions and comparing resulting dicts
@@ -577,6 +573,9 @@ def grab_properties(elt, table):
elt = next


# TODO: The order of arguments here is relevant!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really like the beauty and simplicity of this lexer definitions.
However it relies on ordered keyword arguments, which are not guaranteed for python 3.0 - 3.5.
Nevertheless, personally I would blame python to be buggy here (having changed the semantics).
As this is fixed in python 3.6, I wouldn't go for an ugly workaround.

The issue only arises for documents containing $${ expressions. We could also solve the issue by finding regular expressions that are mutually exclusive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it wasn't that difficult to come up with new regular expressions.
Next commit also fixes some corner-case issues that were failing before ;-)

- made them mutually exclusive, such that we don't face the keyword args ordering issue anymore
- fixed some unhandled corner cases (added appropriate test cases as well)
  - handle $$( additionally to $${
  - handle text in front of $$ or single $
@codebot
Copy link
Contributor

codebot commented Sep 15, 2017

Thanks @rhaschke for adding the Python 3.5 and 3.6 builds to Travis!

@codebot codebot merged commit 00596f7 into ros:lunar-devel Sep 15, 2017
@rhaschke rhaschke deleted the python3 branch September 15, 2017 21:35
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