Manage subscriptions#392
Conversation
|
Is this ready to be reviewed or are you still working on it? |
|
Yes, this is ready to review. It is just lacking some tests. |
|
|
||
| When viewing a list of your multireddits you may initialize a new one: | ||
|
|
||
| ``programming/python+ruby+haskell`` |
There was a problem hiding this comment.
Documentation looks great!
| def from_multireddit(cls, reddit, loader, multi): | ||
| items = iter(multi.subreddits) | ||
| content = cls('My Multireddit: {}'.format(multi.path), items, loader) | ||
| content._multireddit = multi |
There was a problem hiding this comment.
I would rather not make this a private variable if you intend on accessing it from outside of the instance. Would you be opposed to adding this to the SubscriptionContent class initializer?
def __init__(self, name, subscriptions, loader, multireddit=None):
self.name = name
self.multireddit = multireddit
self.order = None
self.query = None
self._loader = loader
self._subscriptions = subscriptions
self._subscription_data = []| if self.config.https_proxy: | ||
| self.http.proxies['https'] = self.config.https_proxy | ||
| self.modhash = None | ||
| self.modhash = '' |
There was a problem hiding this comment.
Any changes to praw should be made in a separate pull request here https://github.com/praw-dev/praw. That way, the praw test suite will be run against the changes and we can document everything that has diverged from the official release. Whenever changes are made to the praw branch, I run scripts/update_package.py which copies the changes over and updates the commit hash.
There was a problem hiding this comment.
Okay. I'll send those changes upstream. I wasn't sure if the older version would be accepting changes.
There was a problem hiding this comment.
They don't, my bad. I meant to link here https://github.com/michael-lazar/praw3
| """Construct an instance of the Multireddit object.""" | ||
| author = six.text_type(author) if author \ | ||
| else json_dict['path'].split('/')[-3] | ||
| else json_dict['path'].split('/')[-4] |
There was a problem hiding this comment.
What is the purpose of this change, is it fixing a bug in praw?
There was a problem hiding this comment.
Yes it's a bugfix:
author = "/user/multi-mod/m/android".split('/')[-4] = "multi-mod"
There was a problem hiding this comment.
>>> "/user/multi-mod/m/android".split('/')[-4]
'user'
There was a problem hiding this comment.
Sorry, I meant this (note the trailing slash):
>>> "/user/multi-mod/m/android/".split('/')[-4]
'multi-mod'
| name = self.term.prompt_input('Subscribe to: /') | ||
| if name is not None: | ||
| with self.term.loader('Adding /r/{}'.format(name, context)): | ||
| for n in name.split('+'): |
There was a problem hiding this comment.
Is supporting the + format really necessary here? I understand using it for initializing multireddits, but I can't imagine that somebody is going to want to subscribe to more than a single subreddit at once. The reason I bring it up is that the error handling could be a lot clearer (show a terminal notification if the subscription fails) if there is only one subreddit. I worry that silently ignoring invalid subreddits could be seen as an error from the end user's perspective.
There was a problem hiding this comment.
Not really necessary. I just wanted to keep the format consistent with the multireddit initialization syntax. I'm fine with removing it.
| name = self.term.prompt_input('Add subreddits(s): /') | ||
| if name is not None: | ||
| with self.term.loader('Adding /r/{}'.format(name)): | ||
| for n in name.split('+'): |
There was a problem hiding this comment.
Same comment here as above
| self.reddit.delete_multireddit(listing.name) | ||
| self.refresh_content() | ||
| else: | ||
| with self.term.loader('Deleting {} from {}'.format(name, context)): |
There was a problem hiding this comment.
The language could be more specific here to avoid confusion - "Unsubscribing {} from {}"
| listing.unsubscribe() | ||
| self.refresh_content() | ||
|
|
||
| @SubscriptionController.register(Command('SUBMISSION_TOGGLE_COMMENT')) |
There was a problem hiding this comment.
In this case, I think we should add a new binding since SUBMISSION_* is reserved for the submission page. It can still be bound to the space bar (0x20) but it should have a separate definition like SUBSCRIPTION_VIEW_MULTIREDDIT.
|
Hey there, |
|
Any news on this ? |
3 participants
No description provided.