Add test for double sticky #470
Conversation
|
Lookin' good. |
|
We haven't released this feature yet, and every time I look at these functions it's not clear what the number value really means. What do you think about calling the argument "bottom" and setting it to false by default for both set and get? Then we fill in the missing pieces. This approach makes testing simpler as we need only test a true or false value. |
|
We just used it because that's the name that Deimorz assigned to it in the ModSupport thread. I'm cool with making it a bool, but setting the sticky should default to bottom=True, because that's the behavior you get on the site when you click "sticky". I think getting the sticky should default to bottom=False, because the top sticky is generally the more important one. Thoughts? Can we compromise? |
|
That sounds fine |
|
Bottom default works for me. |
|
Alright, there we go. That should be a little more intuitive now. |
| """Return a Submission object for the sticky of the subreddit.""" | ||
| return objects.Submission.from_json(self.request_json( | ||
| self.config['sticky'] % six.text_type(subreddit))) | ||
| def get_sticky(self, subreddit, bottom=False): |
There was a problem hiding this comment.
Should bottom default to True here, so calling get_sticky without the arguments returns one that was set via set without the argument? We should probably assert that that is in fact the case.
|
If you make a GET request with no parameters, reddit assumes you wanted the top sticky. For POST, reddit assumes you're replacing the bottom sticky unless you state otherwise. In my opinion, PRAW should act the same way so it is in line with the site and predictable, so I'd like to leave it. I can add an extra note in the docstrings that the two methods behave differently though. |
|
I don't like their inconsistency on this matter and strongly suggest we make our code better than theirs. Many PRAW users don't know the default API behavior, and shouldn't have to. |
|
With bottom=True as the default,
That's not so much a matter of who knows the default API behavior, it's just an unexpected difference between what "sticky this please" means on the site and in praw. You're the owner, so if you insist on the change then I'll go ahead with it. I just want to explain why I feel it should be another way. |
|
Okay, I think I'm getting it. So on reddit sticky-ing one item puts it on top, and all others puts it on bottom replacing the bottom if it exists. From their interface the only way to move something to the top is to remove the top sticky thereby promoting the bottom sticky to the top. Am I right that if there are not stickys, and we add something to the bottom, it really is adding it to the top? Ideally reddit would provide an endpoint that returns a list of the up to two stickies. But until such a time I'm okay with the default Thanks for providing the explanation. |
|
That is all correct. Here's Deimorz' comments:
The rationale being:
So we can't set them both to True without giving people surprise 404s, and can't set them both to False without mismatching the site UI. Last call for changes? |
| @@ -1301,7 +1301,7 @@ def short_link(self): | |||
| return urljoin(self.reddit_session.config.short_domain, self.id) | |||
|
|
|||
| @restrict_access(scope='modposts') | |||
| def sticky(self, num=2): | |||
| def sticky(self, bottom=True): | |||
There was a problem hiding this comment.
The parameter name should be updated. Also we should probably explain what actually happens, e.g., setting bottom when there is no top will make it the top.
|
Okay, I've added some additional notes. Not sure if the wording came out a little clunky, it's hard to use the word "sticky" three times in the same sentence without sounding weird. |
Also added the num parameter to
get_sticky