Skip to content
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

bpo-38870: refactor delimiting with context managers #17612

Open
wants to merge 1 commit into
base: master
from

Conversation

@isidentical
Copy link
Contributor

isidentical commented Dec 15, 2019

return self._NoDelimit()

def delimit(self, delimiter = "()"):
return self._Delimit(self, delimiter)

This comment has been minimized.

Copy link
@vstinner

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.

Copy link
@pablogsal

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.

Copy link
@pablogsal

This comment has been minimized.

Copy link
@vstinner

vstinner Dec 15, 2019

Member

You wouldn't have such issue if ast.unparse would be a submodule....

This comment has been minimized.

Copy link
@pablogsal

pablogsal Dec 15, 2019

Member

I dislike the submodule idea more:

#17377 (comment)

self.write(" := ")
self.traverse(node.value)
self.write(")")
with self.delimit():

This comment has been minimized.

Copy link
@vstinner

vstinner Dec 15, 2019

Member

Maybe it would be more readable if "()" is always passed explicitly. @pablogsal: what do you think?

This comment has been minimized.

Copy link
@pablogsal

pablogsal Dec 15, 2019

Member

I agree, I don't think delimiter should not have a default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.