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

Ensure that channel names don't have spaces #593

Merged
merged 3 commits into from
Sep 16, 2018

Conversation

jamessynge
Copy link
Contributor

Throws an exception if a channel name doesn't match
a regular expression, one which doesn't include spaces.
Fixes issue #205.

Enforce that the value of a message is either a string
or a dict (that was already effectively required, but
now the failure message will be clear).

Greatly expand the class level documentation in PanMessaging.

Throws an exception if a channel name doesn't match
a regular expression, one which doesn't include spaces.
Fixes issue panoptes#205.

Greatly expand the class level documentation in PanMessaging.

Enforce that the value of a message is either a string
or a dict (that was already effectively required, but
now the failure message will be clear).
@jamessynge jamessynge requested a review from wtgee September 15, 2018 16:51
Copy link
Member

@wtgee wtgee left a comment

Choose a reason for hiding this comment

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

Thanks for all the docs, WAY better! I'd like the change made about the preferred usage of create_subscriber with a channel but approving now.


* PANCHAT (sent from POCS.say)
* PAWS-CMD (sent from PAWS websockets.py)
* POCS (sent by class POCS)
Copy link
Member

Choose a reason for hiding this comment

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

I don't recall what this is used for.

pocs/utils/messaging.py Show resolved Hide resolved
* RUNNING (test_pocs.py)
* POCS-CMD (test_pocs.py)

The method receive_message will return messages from all channels;
Copy link
Member

Choose a reason for hiding this comment

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

While this is true, the intended usage is to pass the channel to the create_subcriber and then no filtering is needed by the user. This should be the recommended behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFTER this PR, we should consider using the name topic instead of channel or msg_type, which is the term used by zeromq. It'll make it easier for developers that are trying to understand this subsystem.


Note: PAWS doesn't use PanMessaging, which will likely result in
problems as we evolve PanMessaging and the set of channels.
TODO: Figure out how to share PanMessaging with PAWS.
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 the answer is that we will be changing PAWS.

pocs/utils/messaging.py Outdated Show resolved Hide resolved
@@ -188,13 +259,13 @@ def scrub_message(self, message):
if isinstance(v, Time):
v = str(v.isot).split('.')[0].replace('T', ' ')

# Hmmmm
# Hmmmm. What is going on here? We need some documentation.
Copy link
Member

Choose a reason for hiding this comment

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

Yup. I can't remember what case this was handling. I'll try to find out.

@jamessynge jamessynge merged commit 3b3bfc6 into panoptes:develop Sep 16, 2018
@jamessynge jamessynge deleted the issue-205-blank-free-name branch September 16, 2018 01:17
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.

2 participants