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.

Sign up for GitHub

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

Jump to bottom

Channels - RSN1 to RSN4a #18

Merged
merged 8 commits into from
Aug 5, 2015
Merged

Channels - RSN1 to RSN4a #18

merged 8 commits into from
Aug 5, 2015

Conversation

fjsj
Copy link
Contributor

@fjsj fjsj commented Aug 4, 2015

No description provided.

@mattheworiordan
Copy link
Member

Added a few comments, but overall this looks great.

@fjsj
RSN1 and RSN2
09a32c7 
(RSN1) Channels is a collection of Channel objects accessible through Rest#channels.
(RSN2) Methods should exist to check if a channel exists or iterate through the existing channels
@fjsj
RSN3a
9ec252b 
Creates a new Channel object for the specified channel if none exists, or returns the existing channel. ChannelOptions can be specified when instancing a new Channel.
@fjsj
RSN4a
7edde5c 
Releases the channel resource i.e. it’s deleted and can then be garbage collected
@fjsj
RSN3b
8a31536 
If options are provided, the options are set on the Channel. Accessing an existing Channel with options in the form Channels#get(channel, options) will therefore update the options on the channel and then return the existing Channel
@fjsj
Fixing Channels.__iter__ on Python 3
25bc707
@fjsj
Implementing Channels.release
eb0c308
@fjsj
Removing None var in test_channels_get_doesnt_updates_existing_with_n…
74058d0 
…one_options
@fjsj
Channels.__iter__ should use six.itervalues
266b130
@fjsj
Copy link
Contributor Author

fjsj commented Aug 4, 2015

Fixed the stuff you've mentioned. Sorry, your comments disappeared because I tried a wrong rebase.

EDIT: Your comments were:

  • As discussed, whilst we should support this, I would really prefer we add a release method as well please.
  • It would be nice to see a #get definition as follows instead of passing in None: channel_same = self.ably.channels.get('new_channel')
  • Seems crazy to have to catch the exception every time in Python 3, is there no better way to detect that it's Python 3 as opposed to catching an exception?

mattheworiordan
mattheworiordan reviewed Aug 5, 2015
View reviewed changes
ably/rest/channel.py

def get(self, name, options=None):
if isinstance(name, six.binary_type):
name = name.decode('ascii')

Copy link
Member

Choose a reason for hiding this comment

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

I will merge now, but where we write code to add 2/3 support, it may be nice to add a small comment with an explanation for future us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I'll add a comment in all lines I've changed for Python 3 support

mattheworiordan added a commit that referenced this pull request Aug 5, 2015
@mattheworiordan
Merge pull request #18 from vintasoftware/rsn1-4a
b4c1094 
Channels - RSN1 to RSN4a
@mattheworiordan mattheworiordan merged commit b4c1094 into ably:master Aug 5, 2015
@mattheworiordan
Copy link
Member

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@fjsj @mattheworiordan