Skip to content

Adding comm_info_request and comm_info_reply messages #25

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

Merged
merged 3 commits into from
Sep 7, 2015

Conversation

SylvainCorlay
Copy link
Member

Adding a comm_list_[request/reply] shell message. This requires jupyter/jupyter_client#34.


# Should this be moved to ipkernel?
if hasattr(self, 'comm_manager'):
comm_list = self.comm_manager.comms.keys()
Copy link
Member

Choose a reason for hiding this comment

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

This will presumably need to be cast to a list.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's right. Python 3 sorry.

@SylvainCorlay
Copy link
Member Author

Done

def comm_list_request(self, stream, ident, parent):
content = parent['content']

# Should this be moved to ipkernel?
Copy link
Member

Choose a reason for hiding this comment

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

ipykernel? Actually, we should just answer the question now...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that it is @minrk's call.

Copy link
Member

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 fine place for it.

@SylvainCorlay SylvainCorlay force-pushed the comm_list_shell_message branch from 1e26373 to 5dc4c7d Compare June 27, 2015 22:41
@SylvainCorlay SylvainCorlay changed the title Adding comm_list_request and comm_list_reply messages Adding comm_info_request and comm_info_reply messages Jun 27, 2015
@SylvainCorlay SylvainCorlay force-pushed the comm_list_shell_message branch from 5dc4c7d to 3a57345 Compare July 2, 2015 14:37
@SylvainCorlay
Copy link
Member Author

Rebased to use the new schema for the comm_info_reply.

Message type: ``comm_info_request``::

    content = {
        #optional, (filter by target name)
        'target_name' : str,
    }

Message type: ``comm_info_reply``::

    content = {
        # A dictionary of the comms, indexed by uuids.
        'comms': {
            comm_id: {
                'target_name': str,
            },
        },
    }

@SylvainCorlay SylvainCorlay force-pushed the comm_list_shell_message branch from 3a57345 to c9e1f5e Compare July 13, 2015 22:07
@SylvainCorlay
Copy link
Member Author

I added the ability to filter by target name as discussed earlier.

@minrk
Copy link
Member

minrk commented Aug 13, 2015

thanks, @SylvainCorlay!

@SylvainCorlay
Copy link
Member Author

I hesitated to give a list of target names rather than a single target name for the filter, but it would have been the only message having a list of target names, so it is probably more consistent with the rest of the messages to only allow one.

@minrk
Copy link
Member

minrk commented Aug 14, 2015

@SylvainCorlay good choice. I don't imagine the use case for people wanting more than one but less than all is worth having the extra logic.

def test_comm_info_request():
flush_channels()

msg_id = KC.comm_info()
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a if not hasattr(KC, 'comm_info'): raise SkipTest()? Then this test will run while comm_info is defined, but skip while jupyter_client is < 4.1.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@SylvainCorlay
Copy link
Member Author

(Done)

@minrk
Copy link
Member

minrk commented Sep 7, 2015

@SylvainCorlay thanks! Now I think we can start getting this tested for those living on master.

minrk added a commit that referenced this pull request Sep 7, 2015
Adding comm_info_request and comm_info_reply messages
@minrk minrk merged commit e8809ab into ipython:master Sep 7, 2015
@minrk minrk added this to the 4.1 milestone Sep 7, 2015
@SylvainCorlay SylvainCorlay deleted the comm_list_shell_message branch September 7, 2015 19:14
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.

3 participants