Skip to content
This repository has been archived by the owner on Aug 22, 2019. It is now read-only.

added socketio channel implementation #987

Merged
merged 3 commits into from Sep 13, 2018
Merged

added socketio channel implementation #987

merged 3 commits into from Sep 13, 2018

Conversation

tmbo
Copy link
Member

@tmbo tmbo commented Sep 12, 2018

Proposed changes:

  • added socketio channel implementation

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation
  • updated the changelog

@tmbo tmbo requested a review from akelad September 12, 2018 13:53
Copy link
Contributor

@akelad akelad left a comment

Choose a reason for hiding this comment

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

looks good, two small questions

def __init__(self,
user_message_evt="user_uttered", # type: Text
bot_message_evt="bot_uttered", # type: Text
namespace=None # type: Optional[Text]
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this namespace for? if we're reading it from the credentials file it should also be explained in the docs right?

Copy link
Member Author

Choose a reason for hiding this comment

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

no clue :D didn't read the socket io docs - but i think it allows you to use the same message names in different contexts. don't think it is important though.

def connect(sid, environ):
logger.debug("User {} connected to socketio endpoint.".format(sid))

@sio.on('disconnect', namespace=self.name())
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this self.name() and the connect one above self.namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

good spot, should be namespace as well 👍

@tmbo tmbo merged commit e5a1618 into master Sep 13, 2018
@tmbo tmbo deleted the socketio branch September 13, 2018 20:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants