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

Fix deactivation of embedded instance. #10207

Merged
merged 1 commit into from Jan 29, 2017

Conversation

Projects
None yet
2 participants
@Carreau
Member

Carreau commented Jan 27, 2017

While deactivation of full instances make some sens, the most common
behavior user expect when creating an instance and using
kill_embedded is deactivation of the current call location.

Implement the expected on by default, add options to get previous
behavior, and add flag for direct exit and no-confirm

Fix #9761

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau
Member

Carreau commented Jan 29, 2017

else:
kill = True
if kill:
self.shell._disable_init_location()

This comment has been minimized.

@takluyver

takluyver Jan 29, 2017

Member

Haven't you got these back to front? This is the instance branch, but it looks like it's killing by location (and vice versa).

@takluyver

takluyver Jan 29, 2017

Member

Haven't you got these back to front? This is the instance branch, but it looks like it's killing by location (and vice versa).

This comment has been minimized.

@takluyver

takluyver Jan 29, 2017

Member

Also, since you've added a distinction between call location and init location, do we need ways to kill by both? Or is that starting to get too arcane and fiddly?

@takluyver

takluyver Jan 29, 2017

Member

Also, since you've added a distinction between call location and init location, do we need ways to kill by both? Or is that starting to get too arcane and fiddly?

This comment has been minimized.

@takluyver

takluyver Jan 29, 2017

Member

Aha, I see - setting shell.embedded_active = False is equivalent to killing by call location, because of the property access. I'd forgotten about that bit.

@takluyver

takluyver Jan 29, 2017

Member

Aha, I see - setting shell.embedded_active = False is equivalent to killing by call location, because of the property access. I'd forgotten about that bit.

This comment has been minimized.

@Carreau

Carreau Jan 29, 2017

Member

yeah, it's a bit weird, killing by instance by setting an attribute on the instance does not really help if instances are created in a loop. So I do keep track of instance location and call location.

Honestly I shouldn't have make the "embedded_active" a property and keep all these details private... but backward compatibility. The worst case I'm trying to cover is :

for loop:
    ipembed = InteractiveShellEmbed()
    if option:
         ipembed()
    elseif:
         ipembed()
    else:
         ipembed()

You can either kill the 3 call locations or kill a single instance location.

I agree that the "instance location" is abit weirdish, but that's the behavior of 5.0 and 5.1, so I leave it as an option.

So I think I got the logic right, and I think we should keep both.

@Carreau

Carreau Jan 29, 2017

Member

yeah, it's a bit weird, killing by instance by setting an attribute on the instance does not really help if instances are created in a loop. So I do keep track of instance location and call location.

Honestly I shouldn't have make the "embedded_active" a property and keep all these details private... but backward compatibility. The worst case I'm trying to cover is :

for loop:
    ipembed = InteractiveShellEmbed()
    if option:
         ipembed()
    elseif:
         ipembed()
    else:
         ipembed()

You can either kill the 3 call locations or kill a single instance location.

I agree that the "instance location" is abit weirdish, but that's the behavior of 5.0 and 5.1, so I leave it as an option.

So I think I got the logic right, and I think we should keep both.

Fix deactivation of embedded instance.
While deactivation of full instances make some sens, the most common
behavior user expect when creating an instance and using
``kill_embedded`` is deactivation of the current call location.

Implement the expected on by default, add options to get previous
behavior, and add flag for direct exit and no-confirm

Fix #9761
@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Jan 29, 2017

Member

comments should be addressed (except the call-location/init-location thing where I don't think anything is needed)

Member

Carreau commented Jan 29, 2017

comments should be addressed (except the call-location/init-location thing where I don't think anything is needed)

@takluyver takluyver merged commit 83350fb into ipython:master Jan 29, 2017

4 checks passed

codecov/patch 13.95% of diff hit (target 0.00%)
Details
codecov/project 66.18% (-0.05%) compared to d8f67ff
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 29, 2017

Member

Thanks!

@meeseeksdev backport to 5.x

Member

takluyver commented Jan 29, 2017

Thanks!

@meeseeksdev backport to 5.x

meeseeksdev bot pushed a commit that referenced this pull request Jan 29, 2017

Backport PR #10207: Fix deactivation of embedded instance.
While deactivation of full instances make some sens, the most common
behavior user expect when creating an instance and using
``kill_embedded`` is deactivation of the current call location.

Implement the expected on by default, add options to get previous
behavior, and add flag for direct exit and no-confirm

Fix  9761

@Carreau Carreau deleted the Carreau:fix-embedded branch Jan 29, 2017

Carreau added a commit that referenced this pull request Jan 29, 2017

@Carreau Carreau added the backported label Jan 29, 2017

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