Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-38870: refactor delimiting with context managers #17612
Merged
+137
−147
Conversation
|
Apart open questions to related to https://bugs.python.org/issue39069, this PR looks good to me. @isidentical: Are you interested to measure the overhead of functools/enum imports/code? See https://bugs.python.org/issue39069#msg358498 |
This comment has been minimized.
This comment has been minimized.
|
Yes but currently I can only reply through mail because bpo logins with
google doesnt work. And if needed I can implement this lazy loading idea i
proposed earlier.
…On Mon, Dec 16, 2019 at 9:18 PM Victor Stinner ***@***.***> wrote:
***@***.**** commented on this pull request.
Apart open questions to related to https://bugs.python.org/issue39069,
this PR looks good to me.
@isidentical <https://github.com/isidentical>: Are you interested to
measure the overhead of functools/enum imports/code? See
https://bugs.python.org/issue39069#msg358498
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17612?email_source=notifications&email_token=ALJKHQLANSE4ARV52LDEUKTQY7A6PA5CNFSM4J27S27KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCPK2EHA#pullrequestreview-332767772>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALJKHQJ63VEAVDM5HQZOEATQY7A6PANCNFSM4J27S27A>
.
|
This comment has been minimized.
This comment has been minimized.
|
Oh, looks like @pablogsal already assigned. |
This comment has been minimized.
This comment has been minimized.
Oh, I didn't know: I reported this issue at python/bugs.python.org#41 |
|
I would prefer to only not delimit() if the delimiter is always written, like in a function call or item[index]. |
Lib/ast.py
Outdated
| self.fill(def_str) | ||
| self.traverse(node.args) | ||
| self.write(")") | ||
| with self.delimit("()"): |
This comment has been minimized.
This comment has been minimized.
vstinner
Dec 16, 2019
Member
Wait. I'm not sure about this one. Are you going to omit parenthesis here sometimes?
The change is correct, but direct write() calls are maybe better.
Lib/ast.py
Outdated
| lambda: self.write(", "), write_item, zip(node.keys, node.values) | ||
| ) | ||
| self.write("}") | ||
| with self.delimit("{}"): |
This comment has been minimized.
This comment has been minimized.
vstinner
Dec 16, 2019
Member
Same here, I'm not convince thta delimit() makes the code more readable.
Lib/ast.py
Outdated
| else: | ||
| self.interleave(lambda: self.write(", "), self.traverse, node.elts) | ||
| self.write(")") | ||
| with self.delimit("()"): |
This comment has been minimized.
This comment has been minimized.
Lib/ast.py
Outdated
| comma = True | ||
| self.traverse(e) | ||
| self.write(")") | ||
| with self.delimit("()"): |
This comment has been minimized.
This comment has been minimized.
Lib/ast.py
Outdated
| self.write("[") | ||
| self.traverse(node.slice) | ||
| self.write("]") | ||
| with self.delimit("[]"): |
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Victor Stinner <vstinner@python.org>
This comment has been minimized.
This comment has been minimized.
|
@vstinner a general answer, I used delim in everywhere we are using brackets for the sake of consistency. If it looks better without them, I can just revert back in few places. |
|
@pablogsal: Do you prefer to only use delim() when parenthesis can be optional, or are you fine with replacing all write(x) ... write(y) with "with delim(xy): ..."? |
This comment has been minimized.
This comment has been minimized.
I am fine with replacing all |
Misc/NEWS.d/next/Library/2019-12-15-14-07-16.bpo-38870.8D28DB.rst
Outdated
Show resolved
Hide resolved
|
Almost there! I left some comments (will maybe leave more on a second pass). |
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Dec 23, 2019
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
This comment has been minimized.
This comment has been minimized.
|
I have made the requested changes; please review again |
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Dec 23, 2019
|
Thanks for making the requested changes! @pablogsal: please review the changes made to this pull request. |
|
LGTM |
This comment has been minimized.
This comment has been minimized.
|
I am going to land this one to unblock the rest of the work. @vstinner, we can move the discussion about the readability of the code that you mention to the issue or another PR |
This comment has been minimized.
This comment has been minimized.
|
Thanks for the PR @isidentical! |
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Dec 23, 2019
|
This comment has been minimized.
This comment has been minimized.
|
I guess this is not related with my PR, does it? |
This comment has been minimized.
This comment has been minimized.
Is not, is a core dump on FreeBSD. This is going to be fun.... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
isidentical commentedDec 15, 2019
•
edited by bedevere-bot
CC: @vstinner
https://bugs.python.org/issue38870