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

Added collapse and uncollapse methods to Message object #463

Closed
wants to merge 8 commits into from
Closed

Added collapse and uncollapse methods to Message object #463

wants to merge 8 commits into from

Conversation

danthedaniel
Copy link
Contributor

These endpoints are currently undocumented, although I have made a PR to have reddit add documentation to their /dev/api page:

reddit-archive/reddit#1365

"""

data = {'id': self.name}
url = 'https://api.reddit.com/api/collapse_message/'
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add these urls to the api paths config in the form seen there. Then you can replace these lines with url = self.reddit_session.config['collapse_message']. Just make sure to keep them alphabetized.

@voussoir
Copy link
Contributor

Thanks for finding this! Should be a quick merge with just those few changes and flake8 fixes.

Oh, also make sure to return the json response, even though it'll probably be an empty dictionary.

@bboe
Copy link
Member

bboe commented Jul 12, 2015

It'd be awesome to have tests written for this that utilize the privatemessage scope. Think you're up for giving it a shot?

@danthedaniel
Copy link
Contributor Author

I can see what I can do, but I won't be able to do that right now.

Also - any reason why it's failing the build tests? It's all running fine on my machine and I didn't change very much.

@voussoir
Copy link
Contributor

It might be passing the betamax tests, but there are some flake8 formatting issues. It says here you've got a blank line with some whitespace characters in it, and it doesn't like your docstrings being split up the way they are.

If you do pip install flake8 and then run flake8 in the main praw folder, it should run over everything and tell you what's wrong. There will be a lot of conf errors and init_test_environment errors that don't matter.

@bboe
Copy link
Member

bboe commented Jul 12, 2015

If you open up the p27 test you'll see this information:

0.83s$ flake8 praw
praw/objects.py:710:1: W293 blank line contains whitespace
The command "flake8 praw" exited with 1.
0.57s$ flake8 tests
The command "flake8 tests" exited with 0.
2.03s$ pep257 praw
praw/objects.py:702 in public method `collapse`:
        D202: No blank lines allowed after function docstring (found 1)
praw/objects.py:702 in public method `collapse`:
        D400: First line should end with a period (not 'l')
praw/objects.py:702 in public method `collapse`:
        D200: One-line docstring should fit on one line with quotes (found 3)
praw/objects.py:712 in public method `uncollapse`:
        D202: No blank lines allowed after function docstring (found 1)
praw/objects.py:712 in public method `uncollapse`:
        D400: First line should end with a period (not 'l')
praw/objects.py:712 in public method `uncollapse`:
        D200: One-line docstring should fit on one line with quotes (found 3)

These checks are in place just to help keep things in a consistent manner.

You can run those locally yourself via flake8 praw and pep257 praw. You may need to install those tools to run them: pip install flake8 pep257.

@danthedaniel
Copy link
Contributor Author

"meat"

@voussoir
Copy link
Contributor

Just one last push, please (unless you want to then write tests afterwards). Can you move the collapse and uncollapse methods so they are below the underscore methods? Then, if you can make it so that uncollapse points to collapse in the same way that unhide points to hide and remove_editor uses add_editor, it would be more stylistically consistent. When you do this, you will need to remove the decorator from uncollapse or else it will fail.

Finally, uncollapse_message needs to be above unfriend as bboe pointed out.

Great changes so far.

@danthedaniel
Copy link
Contributor Author

Well this is trickier than I'd anticipated, but thanks for all of the help!

@bboe
Copy link
Member

bboe commented Jul 12, 2015

Thanks for the PR. I'm going to squash up the commits and merge.

@bboe
Copy link
Member

bboe commented Jul 12, 2015

Merged as 0181ac5. Thanks!

@bboe bboe closed this Jul 12, 2015
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.

3 participants