-
-
Notifications
You must be signed in to change notification settings - Fork 453
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
Add additional SubredditFlairTemplate params #1138
Add additional SubredditFlairTemplate params #1138
Conversation
Hi @jackodsteel, The PR failed to pass CI because of an error in the documentation. Here's a link to the CI build. The error is
I believe you can fix the error by replacing
with
The dot is necessary to tell Sphinx that the full path of the class (which happens to be |
11d6f11
to
2f1ce80
Compare
Specifically, added allowable_content and max_emojis params. As discussed as wanted in this Reddit thread by /u/Levi-Ackerbot: https://www.reddit.com/r/redditdev/comments/dy1qw7/is_there_any_way_to_limit_the_amount_of_emojis_in/
2f1ce80
to
828c3d5
Compare
My bad! Checks are passing now @jarhill0 |
Thanks for the PR! Unrelated to your changes, I'm not happy with the existing behavior of these methods, which seems to reset all parameters not passed to their default values. For example, updating the text of a template resets its text color. This behavior predates your changes and I'm not sure what exactly to do about it, but it's unrelated to the scope of this PR. There is one thing I noticed about your changes in playing around with them, which is that I can't seem to set
Here, PRAW is enforcing that I pass the I'll need to take some time later to look over this PR fully, but it looks like it shouldn't be too much trouble to get it merged. |
…ditional-flair-template-params � Conflicts: � CHANGES.rst
The issue you're seeing is that if you set To work successfully first make sure emojis are enabled on your test sub at: Then to add/edit a flair template with So I think it is working as it should, but the API is pretty unclear. I've updated the docs to better explain the requirement. Note that these two commands succeed:
|
Now properly explain that of set to 'emoji' then the 'text' param is constrained to valid emoji strings
I agree about this being a bad behaviour. I think it should be fairly simple to populate the data with the existing then overwrite with new, but like you say that's an issue for another PR. I've opened #1139 to track it. I could probably get to it some time next week, but otherwise if you want to do it feel free. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor modification requests. Otherwise this looks great. Thanks!
2674fa6
to
8f0c462
Compare
Thanks for the pull request! You're awesome. 🎆 |
Specifically, added allowable_content and max_emojis params.
As discussed as wanted in this Reddit thread by /u/Levi-Ackerbot:
https://www.reddit.com/r/redditdev/comments/dy1qw7/is_there_any_way_to_limit_the_amount_of_emojis_in/