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

pyin message now have execution_count #1620

Merged
merged 2 commits into from Apr 18, 2012

Conversation

ivanov
Copy link
Member

@ivanov ivanov commented Apr 18, 2012

This addresses #1619.

One remaining instance of a 'pyin' message that currently lacks execution_count is found on line 112 of IPython/zmq/pykernel.py - I'm not sure when this code path is used, and what the appropriate way to proceed there is.

@minrk, I'm ping-ponging this one back at you :)

@fperez
Copy link
Member

fperez commented Apr 18, 2012

Looks clean to me, will also give time so others can review. Thanks!

@minrk
Copy link
Member

minrk commented Apr 18, 2012

Don't worry about pykernel. ~nobody uses it, and it should just be removed (I've been working on merging the Kernel implementations for Engines/Kernels, and that branch removes pykernel).

@minrk
Copy link
Member

minrk commented Apr 18, 2012

This looks good to me, +1 to merge.

@fperez
Copy link
Member

fperez commented Apr 18, 2012

Great, merging now. Thanks @ivanov !

@fperez
Copy link
Member

fperez commented Apr 18, 2012

Actually, I just realized we have that other codepath to deal with, @ivanov... What do you want to do about it? Do you want to have a go at fixing it there as well? We might as well leave this one clean so we can close #1619...

@minrk
Copy link
Member

minrk commented Apr 18, 2012

@fperez pykernel has always violated the message spec all over the place, and we should just remove it. I wouldn't bother adding this to it.

@minrk
Copy link
Member

minrk commented Apr 18, 2012

If you do want to fix it in pykernel: the value should just be 0 or None, because execution count is not defined for a non-IPython kernel. but my vote is to simply ignore it and merge as-is.

@fperez
Copy link
Member

fperez commented Apr 18, 2012

Ah, hold on; I failed to realize that the problem was in pykernel, which @minrk already said is slated for the guillotine. Let's just merge as-is. I'll do so right now, thanks everyone! (and sorry for my confusion)

fperez added a commit that referenced this pull request Apr 18, 2012
Set the `execution_count` field for pyin messages.

Closes #1619.
@fperez fperez merged commit 79d7b1c into ipython:master Apr 18, 2012
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Set the `execution_count` field for pyin messages.

Closes ipython#1619.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants