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
Conversation
| return self._NoDelimit() | ||
|
|
||
| def delimit(self, delimiter = "()"): | ||
| return self._Delimit(self, delimiter) |
This comment has been minimized.
This comment has been minimized.
vstinner
Dec 15, 2019
Member
IHMO a contextlib.contextmanager would be simpler. Something like:
@contextlib.contextmanager
def delim_if(self, condition, delimiter="()"):
if condition:
self.write(self.delimiter[0])
try:
yield
finally:
if condition:
self.write(self.delimiter[-1])
# I'm not sure that this one works as expected :-)
def delim(self, delimiter):
return self.delim_if(True, delimiter)
This comment has been minimized.
This comment has been minimized.
pablogsal
Dec 15, 2019
Member
We started with this, but it introduced a regression in the import time and I reverted it.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
vstinner
Dec 16, 2019
Member
This discussion is splitted between 3 comment threads in 2 PRs. I created https://bugs.python.org/issue39069 to summarize the discussion and discuss the idea there.
|
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]. |
| 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.
| 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.
| 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.
| comma = True | ||
| self.traverse(e) | ||
| self.write(")") | ||
| with self.delimit("()"): |
This comment has been minimized.
This comment has been minimized.
| 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): ..."? |
isidentical commentedDec 15, 2019
•
edited by bedevere-bot
CC: @vstinner
https://bugs.python.org/issue38870