-
Notifications
You must be signed in to change notification settings - Fork 888
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
turn the Configurator into a context manager #2874
Conversation
return self | ||
|
||
def __exit__(self, exc_type, exc_value, exc_traceback): | ||
self.end() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to do any clean-up, or mark the configuration instance as b0rked, if the with
block exits with an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error will just propagate up since we aren't returning True
to squash it. I can add a test to clarify but that's the context manager contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
def __exit__(self, exc_type, exc_value, exc_traceback):
if exc_type is not None:
# self.abort() # we don't have an 'abort()' method.
self.action_state = ActionState()
else:
self.commit()
self.end()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you want to signify by clearing the action state? Is it telling the user that there's a way to try stuff out and rollback?
I like the idea. Practically I think it's dangerous to claim we can do anything about rollbacks. We generally expect any exceptions at config-time to kill your app and do not document error handling for any directives. Also much of the third party extensions I've ever seen do all their work in the directive without deferring it to the commit cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you want to signify by clearing the action state?
Just that any work set up to be done within the scope of the with
block is cancelled.
Also much of the third party extensions I've ever seen do all their work in the directive without deferring it to the commit cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've thought a bit more about this and I think adding a config.commit()
is the right answer here but it would probably be nice if we can avoid doing a commit if it's embedded in another context manager. @tseaver Good idea? Bad idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of any sane way to detect that we are inside another context manager. Maybe we could add an opt-out parameter to the Configurator
? E.g.:
if __name__ == '__main__':
with Configurator() as outer:
# do stuff with outer
with Configurator(commit_on_exit=False) as inner:
inner.add_route('hello', '/hello/{name}')
inner.add_view(hello_world, route_name='hello')
# do more stuff with outer
app = outer.make_wsgi_app()
server = make_server('0.0.0.0', 8080, app)
server.serve_forever()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be done similar to how we handle config.route_prefix
and a few other state variables where we shuffle them through each new configurator. We could actually use that state to warn a user if they try to do a manual commit as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were the comments here addressed? Or did we decide not to move forward with some of the comments here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided that tracking and avoiding nested commits wasn't our problem and was more trouble than it's worth.
The only other issue was clearing the action state which we do not do on normal commit failures nor do we have an api to do it in general and as such I won't be adding it to this type of commit either until we decide to support it in a standardized way across all commit mechanisms.
One consideration is adding an implicit |
2c712a6
to
ac99750
Compare
ac99750
to
134ef7e
Compare
@tseaver I've updated this per your comments to perform a commit. I decided to avoid any complex nesting logic and simply added a warning to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I think this is a great change and simplifies using the configurator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @tseaver and @bertjwregeer . |
The main goal here is to provide a common pattern for calling
config.begin()
andconfig.end()
such that the registry, settings, asset overrides, etc are all properly available at config-time. It's rare right now for apps to be invokingconfig.begin()
and it would be good to improve on this situation.Unfortunately the context manager is not a silver bullet. For example the
config
object in the below example still has a strong ref to it outside the scope of thewith
-statement, but at least its intended lifetime is more clear perhaps.Note the new hello world example:
fixes #2872