bpo-38870: refactor delimiting with context managers#17612
Merged
pablogsal merged 6 commits intopython:masterfrom Dec 23, 2019
Merged
bpo-38870: refactor delimiting with context managers#17612pablogsal merged 6 commits intopython:masterfrom
pablogsal merged 6 commits intopython:masterfrom
Conversation
vstinner
reviewed
Dec 15, 2019
vstinner
reviewed
Dec 16, 2019
Member
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
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
Member
Author
|
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>
.
|
Member
Author
|
Oh, looks like @pablogsal already assigned. |
Member
Oh, I didn't know: I reported this issue at python/bugs.python.org#41 |
vstinner
reviewed
Dec 16, 2019
Member
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
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("()"): |
Member
There was a problem hiding this comment.
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("{}"): |
Member
There was a problem hiding this comment.
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("()"): |
Lib/ast.py
Outdated
| comma = True | ||
| self.traverse(e) | ||
| self.write(")") | ||
| with self.delimit("()"): |
Lib/ast.py
Outdated
| self.write("[") | ||
| self.traverse(node.slice) | ||
| self.write("]") | ||
| with self.delimit("[]"): |
Co-Authored-By: Victor Stinner <[email protected]>
Member
Author
|
@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. |
vstinner
reviewed
Dec 17, 2019
Member
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
@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): ..."?
Member
I am fine with replacing all |
pablogsal
reviewed
Dec 23, 2019
pablogsal
reviewed
Dec 23, 2019
Misc/NEWS.d/next/Library/2019-12-15-14-07-16.bpo-38870.8D28DB.rst
Outdated
Show resolved
Hide resolved
pablogsal
requested changes
Dec 23, 2019
Member
pablogsal
left a comment
pablogsal
left a comment
There was a problem hiding this comment.
Almost there! I left some comments (will maybe leave more on a second pass).
|
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 |
…imiter, remove news entry
…to bpo-38870-delimit
Member
Author
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @pablogsal: please review the changes made to this pull request. |
pablogsal
reviewed
Dec 23, 2019
pablogsal
reviewed
Dec 23, 2019
Member
|
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 😉 |
Member
|
Thanks for the PR @isidentical! 🎉 |
|
Member
Author
|
I guess this is not related with my PR, does it? |
Member
Is not, is a core dump on FreeBSD. This is going to be fun.... |
Member
|
Thanks @isidentical and @pablogsal: the commit 4b3b122 is nice! I will to implement smarter parenthesis. @isidentical: ping me if you want a review on a "smarter parenthesis" PR, once you will have it written down (I didn't check if it's already the case). |
Member
|
@pablogsal: "Is not, is a core dump on FreeBSD. This is going to be fun...." Did you open an issue to track this crash? |
Member
Nop...I started vacation that day and I never did 😞 |
Member
Author
Yes I rebased it a top of this PR (and just fixed some conflicts due to other PR of mine), @vstinner. You can review |
Member
|
Oh, it's the PR #17377, ok. |
shihai1991
pushed a commit
to shihai1991/cpython
that referenced
this pull request
Jan 31, 2020
…ythonGH-17612) Co-Authored-By: Victor Stinner <[email protected]> Co-authored-by: Pablo Galindo <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CC: @vstinner
https://bugs.python.org/issue38870