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

Support link_flair endpoint #815

Merged
merged 1 commit into from Jun 20, 2017
Merged

Conversation

leviroth
Copy link
Contributor

Fixes #812.

I tried a couple different approaches to getting rid of the is_link parameters. I wanted to keep it around since existing code might do something like:

subreddit.flair.templates.clear(is_link=True)

The latest commit removes a lot of code duplication, and it seemed to me like the best approach.

than a Redditor flair template (default: False).
:param is_link: (boolean) When None, choose a template type in a
subclass-defined way. Otherwise, add a link flair template when
True and a Redditor flair template when false (default: None).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This docstring has the potential to be fairly confusing. Without the latest commit, we could just have separate docstrings in different subclasses, but at the cost of a lot of duplication.

@leviroth leviroth force-pushed the link-flair-templates branch 2 times, most recently from 5a57a0d to 949139e Compare June 18, 2017 00:16
Copy link
Member

@bboe bboe left a comment

Choose a reason for hiding this comment

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

For the time being can we make private methods in the parent class, e.g., _add, and _clear which take in the necessary arguments and do the actually work.

Then we have add, and clear precisely defined in each non-abstract class with no need for an is_link attribute.

The gotcha, is that we should release a minor version of PRAW with is_link deprecated for the existing behavior. Then we can remove it as part of PRAW5.

But for the purposes of this PR let's ignore that part and simply clean it up. I'm not worried about people on the development version having things break (that's to be expected).

@leviroth
Copy link
Contributor Author

Is this what you had in mind?

@bboe
Copy link
Member

bboe commented Jun 20, 2017

Is this what you had in mind?

Yes, awesome. Can you add the changelog entry for the addition of the link template, and a removal for the support of the is_link parameter?

@leviroth
Copy link
Contributor Author

Added and squashed.

@bboe bboe merged commit 1c574b6 into praw-dev:master Jun 20, 2017
@bboe
Copy link
Member

bboe commented Jun 20, 2017

Wonderful work thanks. I'll work pulling this into a backport in order to properly deprecate the is_link parameters.

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