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

Settings Context Manager and clean_revert #617

Closed
streeter opened this Issue Apr 13, 2012 · 4 comments

Comments

Projects
None yet
2 participants
@streeter

streeter commented Apr 13, 2012

I found that following example throws an unhandled KeyError exception:

def show_failure():
    env.original = "outer"
    with settings(original="inner", noexist="inner", clean_revert=True):
        env.original = "inner"
    # Exiting the context manager throws a KeyError because 'noexist' does
    # not exist in the original environment

Here's the stacktrace of the exception:

File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py", line 24, in __exit__
    self.gen.next()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py", line 121, in nested
    if exit(*exc):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py", line 24, in __exit__
    self.gen.next()
  File "/Users/streeter/.virtualenvs/fabric/lib/python2.7/site-packages/fabric/context_managers.py", line 106, in _setenv
    state.env[key] = previous[key]
KeyError: 'noexist'

The issue is that the exit handler for the context manager wants to restore the previous value of the key here. However, that key doesn't exist in the previous environment.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Apr 13, 2012

Hm, actually. Need to update this -- the "new" key should not persist outside the block, but it will now. Derp. EDIT: fixed in c533ee3

@bitprophet

This comment has been minimized.

Member

bitprophet commented Apr 13, 2012

By the way, thanks for the catch @streeter!

@streeter

This comment has been minimized.

streeter commented Apr 13, 2012

Sure thing. I'd have sent a pull request too, but wasn't sure how you would want to fix it.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Apr 13, 2012

I think it's good now. Waffled a bit because the point of clean_revert is to allow explicit env changes to persist outside the block -- but in the example you gave, the key is specified in the settings() call itself (implying the change should be temporary/limited to the block) so I figured that yes, it should get cleaned up even when clean_revert is on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment