-
Notifications
You must be signed in to change notification settings - Fork 706
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
Core: Add toggles_as_bools to options.as_dict #3770
Core: Add toggles_as_bools to options.as_dict #3770
Conversation
Seems nice to have to me This is more of an "opinion" PR so like, probably need a couple more of those |
Co-authored-by: Doug Hoskisson <beauxq@users.noreply.github.com>
If we're going to this a kwarg to return current_key would be good too. I'm a bit against doing this because bools are more expensive to encode in slot_data than ints are, but the measurable difference is so minor and they're more convenient to use so eh. But ints are just as easy to parse on the client side. |
You could add that in another PR, I feel it's out of scope here, especially since As far as the expensiveness of bools go, plenty of worlds are already using them, so if AP wanted to prevent that, they should have done so earlier |
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.
I don't see problems with code.
I didn't test it.
I don't have an opinion on whether the feature should exist or not.
Oh wait, technically I was supposed to let Medic merge this themself now that they're a Core Maintainer... oh well :D |
What is this fixing or adding?
It's pretty common for a world to want their Toggle options to be Booleans and not Ints in slot_data, so this adds that as an option.
Might as well improve the typing while we're at it
How was this tested?
Generations and printing slot_data using
options.as_dict
with Toggle and DefaultOnToggle options.Apparently PyCharm also said no more spaces in empty lines 🤷