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

Praw 19 November -- Early support for Multireddits #345

Closed
wants to merge 7 commits into from
Closed

Praw 19 November -- Early support for Multireddits #345

wants to merge 7 commits into from

Conversation

voussoir
Copy link
Contributor

New :class:Multireddit for multireddit objects. Proper endpoints and kind added to init config

New :meth:get_multireddit to get a single mr object. Standard usage: r.get_multireddit(multi_author, multi_name)

New :meth:get_my_multis which requires logged in scope. Returns a list of all multireddit objects you own.

New :class:MultiredditTest in tests which will test the get_my_multis function. As of writing, :class:BasicTest will test get_multireddit since that does not require logged in.

Added proper material to CHANGES.rst

Edit: Travis build failed but I see the error and will re-commit.

Sorry for the commit-flood :( I will learn to squash before next time.

Edit: Build 498 failed on account of test_scope_submit timing out (Line 415). Can a new build be initiated without a commit?

Added Multireddit class to objects.py, added multireddit api endpoints
to init's config, added get_multireddit and get_my_multis, which is not
100% functional yet.
get_multireddit and get_my_multis are now fully functional. Added
LabeledMulti as a recognized kind. New class MultiredditTest will test
get_my_multis, existing class BasicTest will test get_multireddit
Fixed typo in CHANGES.rst
Added comment to __init__.py regarding why we don't use get_content for
get_my_multireddits.

Cleaned up pep8
Added a forward slash to the end of the multireddit_about endpoint even
though it may not be necessary. Fixed the docstring of get_my_multis to
remove references to the get_content function.
Fixed test on get_multireddit which caused travis build to fail.
@@ -124,10 +124,13 @@ class Config(object): # pylint: disable-msg=R0903, R0924
'morechildren': 'api/morechildren/',
'my_con_subreddits': 'subreddits/mine/contributor/',
'my_mod_subreddits': 'subreddits/mine/moderator/',
'my_multis': '/api/multi/mine/',
Copy link
Member

Choose a reason for hiding this comment

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

Leading '/' not required on these added paths.

Parameters for getting multireddits are now called "redditor" and
"multi" across the board. Redditor objects have helper function
get_multireddit to get one of their multis. Added test cases for
Redditor.get_multireddit and Multireddit.get_new. Handled /me/m/
redirect.
@voussoir
Copy link
Contributor Author

>Remove all these functions that do not have explicit tests. I assume many of them 
 don't work for multireddits

Are you referring to the _get_sorters here? I tried every one manually and confirmed in-browser, they all work perfectly. My only problem with them is that they are a waste of lines since I copied them straight from Subreddit. One alternative would be to put those in a separate class then have Subreddit and Multireddit inherit, right?

I have made the other changes you asked, and added Redditor.get_multireddit.

When navigating to a subreddit you own, you are redirected to reddit.com/me/m/multi, this was raising an error in get_content because Multireddit._url is in /user/redditor/m/multi format. I felt that the best solution was to do a check and swap before request_json. At first, I initialized Multireddits with a /me/ url if it belonged to the user, but this would have caused problems if the session was later logged into a different user. Please critique.

Edit: Build 500 failed due to Timeout in test_get_random_subreddit (Line 207).

Edit2: Thanks

@voussoir
Copy link
Contributor Author

Your latest commit has created conflicts with this pull. Should I resolve those or do you manage that during the pull process?

Are there any other changes you were waiting for me to make?

@bboe
Copy link
Member

bboe commented Nov 23, 2014

I'll take care of the merge issues. Thanks!

@bboe
Copy link
Member

bboe commented Nov 23, 2014

Merged as 434d6ab. Thanks!

@bboe bboe closed this Nov 23, 2014
@voussoir
Copy link
Contributor Author

Great! I'll get started on the features such as creation, copying, deletion, etc.

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.

None yet

2 participants