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

add js kernel_info request #4847

Merged
merged 5 commits into from Jan 24, 2014
Merged

add js kernel_info request #4847

merged 5 commits into from Jan 24, 2014

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Jan 22, 2014

I would have hopped it to respond also with a "Kernel Idle".

Would it make sens/is it possible to add the (optional) "status" field to kernel_info_reply ?

@takluyver
Copy link
Member

Can't the kernel only answer the info request when it's not busy? So it dosen't make much sense to have the idle/busy status in the reply.

@minrk
Copy link
Member

minrk commented Jan 23, 2014

Can't the kernel only answer the info request when it's not busy?

Right - it makes no sense for kernel_info_reply to include status, because if there is a reply, the status is idle. So you could do it that way - start the status as busy, and then set it to idle when you get the kernel reply.

@Carreau
Copy link
Member Author

Carreau commented Jan 23, 2014

Right - it makes no sense for kernel_info_reply to include status, because if there is a reply, the status is idle.

Is that obviously true for non-python kernel ? Couldn't you have a kernel that is able to treat
kernel info request independently than executing code ? Cause the requested info are totally unrelated to the current state of what is beeing run.

But agreed for Python kernel. Will update other PR.

Does this prevent the addition of this utility function to the javascript ?

@minrk
Copy link
Member

minrk commented Jan 23, 2014

Does this prevent the addition of this utility function to the javascript?

Not at all. If you add a test, I think this is ready to merge.

@Carreau
Copy link
Member Author

Carreau commented Jan 23, 2014

Should be tested to at least return a dict.

Envoyé de mon iPhone

Le 23 janv. 2014 à 18:26, Min RK notifications@github.com a écrit :

Does this prevent the addition of this utility function to the javascript?

Not at all. If you add a test, I think this is ready to merge.


Reply to this email directly or view it on GitHub.

@@ -0,0 +1,28 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rename to kernel_info.js?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I hope it will contain other kernel tests at some point ... But I can.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just seems weird to have js in the name, so I would remove that at least, and leave it as kernel.js or kernel_test,js

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just seems weird to have js in the name, so I would remove that at least, and leave it as kernel.js or kernel_test,js

Done as soon as git push achieve to go through my connection.

@ghost ghost assigned ellisonbg Jan 23, 2014
@minrk
Copy link
Member

minrk commented Jan 24, 2014

Thanks.

minrk added a commit that referenced this pull request Jan 24, 2014
@minrk minrk merged commit d13647d into ipython:master Jan 24, 2014
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@Carreau Carreau deleted the js-kernel-info branch December 15, 2014 16:47
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

4 participants