GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign up|
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 |
|
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>
.
|
|
Oh, looks like @pablogsal already assigned. |
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]. |
| self.fill(def_str) | ||
| self.traverse(node.args) | ||
| self.write(")") | ||
| with self.delimit("()"): |
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.
| lambda: self.write(", "), write_item, zip(node.keys, node.values) | ||
| ) | ||
| self.write("}") | ||
| with self.delimit("{}"): |
Same here, I'm not convince thta delimit() makes the code more readable.
| else: | ||
| self.interleave(lambda: self.write(", "), self.traverse, node.elts) | ||
| self.write(")") | ||
| with self.delimit("()"): |
| comma = True | ||
| self.traverse(e) | ||
| self.write(")") | ||
| with self.delimit("()"): |
| self.write("[") | ||
| self.traverse(node.slice) | ||
| self.write("]") | ||
| with self.delimit("[]"): |
Co-Authored-By: Victor Stinner <vstinner@python.org>
|
@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): ..."? |
I am fine with replacing all |
|
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 |
|
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. |
|
LGTM |
|
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 |
|
Thanks for the PR @isidentical! |
|
|
I guess this is not related with my PR, does it? |
Is not, is a core dump on FreeBSD. This is going to be fun.... |
isidentical commentedDec 15, 2019
•
edited by bedevere-bot
CC: @vstinner
https://bugs.python.org/issue38870