Skip to content
This repository has been archived by the owner on Jul 5, 2023. It is now read-only.

Implement underscores in numeric literals #21

Merged
merged 5 commits into from
Oct 31, 2016

Conversation

ilevkivskyi
Copy link
Member

FWIW, here is the implementation of underscores in numeric literals. I just cherry-picked the necessary parts from the original implementation in Python 3.6 by Georg and Serhiy. This could be useful for mypy.

@ddfisher Please take a look.

@ilevkivskyi
Copy link
Member Author

@ddfisher Fixed the bug with fractional part, sorry for not noticing it earlier.

@ddfisher
Copy link
Collaborator

ddfisher commented Oct 29, 2016

Sorry for being slow to review this! I was hoping to get to it this week but had some other things take precedence. It's at the top of my list for next week!

Edit: Nevermind! I found time this evening.

Copy link
Collaborator

@ddfisher ddfisher left a comment

Choose a reason for hiding this comment

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

Thanks! This PR looks great overall -- I just have one minor nitpick.

It was difficult for me to evaluate your tokenizer changes, even when comparing them to the commit to CPython (because that change also introduced a bunch of unrelated open and close braces). I ran this on a large codebase and didn't see any problems, so I'm going to assume you've made equivalent changes.

@@ -70,7 +70,7 @@ module Python
-- x < 4 < 3 and (x < 4) < 3
| Compare(expr left, cmpop* ops, expr* comparators)
| Call(expr func, expr* args, keyword* keywords)
| Num(object n) -- a number as a PyObject.
| Num(object n, int? underscores) -- a number as a PyObject.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'd be a bit clearer if this was named contains_underscores instead. Also, could you add a comment noting that this field is not part of standard Python and its purpose is to signal that a newer Python version feature was used?

@ilevkivskyi
Copy link
Member Author

@ddfisher Thank you for review!
I just pushed a new commit with requested changes. I will make small PR with the corresponding change to mypy right now.

I ran this on a large codebase and didn't see any problems, so I'm going to assume you've made equivalent changes

I used both positive and negative example numeric literals from original Python test_grammar and those pass/fail as expected.

@ddfisher
Copy link
Collaborator

Great, thanks!

@ddfisher ddfisher merged commit 93d4e80 into python:master Oct 31, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants