-
Notifications
You must be signed in to change notification settings - Fork 701
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
LTTP: sort of use new options system #3764
Conversation
LttP: use World.random
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.
pure code review, did not run gens/tests/etc.
options dataclass looks good,
did some testing around the old options re-establisher format in a python shell and checked docs and it all looks functional
of note though, this will override the deprecateDicts that core writes, so opening up potential issues of crashing weird if other worlds share that option name and use the old api, not being able to track well what lttp options are being got through multiworld.option_name still, etc.
but I believe that was known when this bandaid was proposed so still approving it
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'm approving this but I think I'd like this to be the last thing we merge before actually hard deprecating the options system, because of that thing qwint mentioned
Which, as of a few seconds ago, this is the only PR remaining. So, whenever we wanna merge #3284, we can merge this first and then that |
What is this fixing or adding?
Makes LttP use the new options system in a way it should continue working when the old one is gone.
How was this tested?
I made deprecatedict raise on error and got a generation