-
Notifications
You must be signed in to change notification settings - Fork 3
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
Accept empty input to SubredditConfig constructor #66
Accept empty input to SubredditConfig constructor #66
Conversation
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.
Thanks so much for the PR! This is really helpful, and the implementation looks good to me. (and thank you for including tests, you are a better dev than me most of the time lmao.)
cannot believe we actually just use empty strings as default config values, that's just... ugh. Your solution works fine for now even if it has some typing issues; I have ideas to make this better in the future but I'm comfortable rolling with this for now.
Posting a couple style nits and some suggestions/clarifications but I don't think this PR needs much before I'm happy merging it.
Thoughts on the defaults problem for future reference - nothing I'm expecting you to do for this PR
I think in the future the best way to handle things will be to have the SubredditConfig
class store data in a completely different structure than the wiki page expects, and just reconstruct the wiki format from the stored information later. This is what the Usernotes
class does currently - it doesn't hang onto any RawUsernotes
data after being constructed, it parses what it's given into its own internal users
structure and converts it back to RawUsernotes
as required. Config is a much simpler structure conceptually, so we can get away with not bothering with that for now, but in the future I'd prefer the SubredditConfig
class to be an actual abstraction rather than just a thin wrapper that just exposes the raw data on a public property.
Co-authored-by: Erin <[email protected]>
Co-authored-by: Erin <[email protected]>
Gonna wait until I'm off work to approve but the updated changes look good at first glance, thanks! |
… fails Co-authored-by: Erin <[email protected]>
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.
sorry i forgor
Closes #58
I decided to handle an empty wiki page by returning the exact values that would be used by the Toolbox plugin, and copied the defaults from the reddit-moderator-toolbox repo here. Doing this means that the results of passing in an empty string vs. loading a config that has literally had nothing done to it are exactly the same.
This is slightly messy, because the defaults set empty strings for everything which actually conflicts with the RawSubredditConfig type. But I felt that this was perhaps not the time to mess with that (especially on my first PR into toolbox-devvit).
I went slightly further than the approach taken in #63, and additionally handle a situation where the
toolbox
page is entirely absent (something I discovered in the real world in one of my Devvit Apps, which is why I decided to pick this issue up). It'd be nice to be able to take out my own code working around this scenario and get it built into the package.I did not attempt to set defaults for the values beyond what Toolbox would set up on the page in the first place (e.g. not setting non-falsy defaults for the removal reasons, macros and so on) because I feel that it would make a lot of sense to implement those at the point of providing specific functions to return those things, just like it's been done for the note types