-
Notifications
You must be signed in to change notification settings - Fork 310
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
refactor: simplify OrderedDict arguments in lexer #598
Conversation
Python 3.6+ guarantees that kwargs order is preserved, thus we don't need to assure the order by passing them as a list of tuples.
# Token definition order is important, thus an OrderedDict is used. In addition, PEP 468 | ||
# guarantees us that the order of kwargs is preserved in Python 3.6+. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note - we still need to use OrderedDict instad of a regular dict, because the regular dict's key insertion order should be considered an implementation detail of CPython 3.6 (that behavior has only been promoted to a language spec in Python 3.7).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! I didn't realize kwargs became ordered a version earlier :-)
@jimfulton Assigned to you if you have free cycles and want to get more familiar with the BigQuery code. |
This is true for CPython, but I recall that the language standard only makes this the case for Python 3.7+. Not that we support/test with alternative runtimes... Maybe worth bringing up in the Python meeting today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much cleaner!
Closes #599. 🦕
Python 3.6+ guarantees that kwargs order is preserved, thus we don't need to assure the order by passing the arguments as a list of tuples (we don't support Python 3.5 anymore). Results in a more compact and cleaner source (IMO).
There are still some
OrderedDict
instances in tests that could benefit from the same, but I'm not touching that to not cause merge conflicts with the tests refactoring changes @jimfulton is making right now.PR checklist