Options: Always verify keys for VerifyKeys options #3280
Conversation
github-actions
bot
added
affects: core
alwaysintreble
added
the
is: bug/fix
|
Haven't tested it, but this looks and feels correct to me |
There was a problem hiding this comment.
I can approve the stardew test changes, not sure about the core changes. I don't have an opinion as to if it's better than #3274
|
|
||
| def verify(self, world: typing.Type[World], player_name: str, plando_options: "PlandoOptions") -> None: | ||
| self.verify_keys() |
There was a problem hiding this comment.
Could we get a docstring on this function so implementers of custom option types know what actually has to be implemented here? I'm still confused by why a method named verify() is mutating itself, and I think the fact that its method of returning an error is to raise an exception rather than having a return value is also of note. Is the exception error message user-facing? When is this function called during generation? Do I have to call it manually in my options unit tests? etc.
There was a problem hiding this comment.
this is a bug fix, docs are extremely out of scope.
https://github.com/ArchipelagoMW/Archipelago/blob/main/Generate.py#L411
the only modifications that verify does is unfolding item and location groups into item and location names to actually be used by generation later.
There was a problem hiding this comment.
That's fair. Again, good to call this comment thread resolved.
Is there an issue / discord thread somewhere where the docs are in the works / where I can ask these questions in general? Ideally I'd think there would be a separate function that would mutate / preprocess / expand out groups in a setting, and this would be documented somewhere (as well as whether that function runs before or after verify). I don't want to clutter up a simple bugfix PR with design / doc talk, though.
There was a problem hiding this comment.
nothing specific but that's what ap-world-dev is for
Exempt-Medic
added
waiting-on: core-review
What is this fixing or adding?
alternative to #3274 which just makes sure that verify_keys() is always called for VerifyKeys subclasses instead of just randomly?
How was this tested?
https://discord.com/channels/731205301247803413/731214280439103580/1237762707550371960
If this makes graphical changes, please attach screenshots.