Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Options: Always verify keys for VerifyKeys options #3280

Merged
merged 6 commits into from
Jul 31, 2024
Merged

Options: Always verify keys for VerifyKeys options #3280

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8fd6d97
Options: Always verify keys for VerifyKeys options
alwaysintreble May 8, 2024
4d3f8cb
Merge branch 'main' into always_verify_keys
NewSoupVi Jul 25, 2024
8e59e90
fix PlandoTexts
alwaysintreble Jul 25, 2024
857962e
use OptionError and give a slightly better error message for which op…
alwaysintreble Jul 25, 2024
90c1671
add the player name to the error
alwaysintreble Jul 25, 2024
ac16957
don't create an unnecessary list
alwaysintreble Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions Options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,17 +786,17 @@ class VerifyKeys(metaclass=FreezeValidKeys):
verify_location_name: bool = False
value: typing.Any

@classmethod
def verify_keys(cls, data: typing.Iterable[str]) -> None:
if cls.valid_keys:
data = set(data)
dataset = set(word.casefold() for word in data) if cls.valid_keys_casefold else set(data)
extra = dataset - cls._valid_keys
def verify_keys(self) -> None:
if self.valid_keys:
data = set(self.value)
dataset = set(word.casefold() for word in data) if self.valid_keys_casefold else set(data)
extra = dataset - self._valid_keys
if extra:
raise Exception(f"Found unexpected key {', '.join(extra)} in {cls}. "
f"Allowed keys: {cls._valid_keys}.")
raise Exception(f"Found unexpected key {', '.join(extra)} in {self}. "
alwaysintreble marked this conversation as resolved.
Show resolved Hide resolved
f"Allowed keys: {self._valid_keys}.")

def verify(self, world: typing.Type[World], player_name: str, plando_options: "PlandoOptions") -> None:
self.verify_keys()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing specific but that's what ap-world-dev is for

if self.convert_name_groups and self.verify_item_name:
new_value = type(self.value)() # empty container of whatever value is
for item_name in self.value:
Expand Down Expand Up @@ -833,7 +833,6 @@ def __init__(self, value: typing.Dict[str, typing.Any]):
@classmethod
def from_any(cls, data: typing.Dict[str, typing.Any]) -> OptionDict:
if type(data) == dict:
cls.verify_keys(data)
return cls(data)
else:
raise NotImplementedError(f"Cannot Convert from non-dictionary, got {type(data)}")
Expand Down Expand Up @@ -879,7 +878,6 @@ def from_text(cls, text: str):
@classmethod
def from_any(cls, data: typing.Any):
if is_iterable_except_str(data):
cls.verify_keys(data)
return cls(data)
return cls.from_text(str(data))

Expand All @@ -905,7 +903,6 @@ def from_text(cls, text: str):
@classmethod
def from_any(cls, data: typing.Any):
if is_iterable_except_str(data):
cls.verify_keys(data)
return cls(data)
return cls.from_text(str(data))

Expand Down
4 changes: 2 additions & 2 deletions worlds/stardew_valley/test/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from contextlib import contextmanager
from typing import Dict, ClassVar, Iterable, Tuple, Optional, List, Union, Any

from BaseClasses import MultiWorld, CollectionState, get_seed, Location, Item, ItemClassification
from BaseClasses import MultiWorld, CollectionState, PlandoOptions, get_seed, Location, Item, ItemClassification
from Options import VerifyKeys
from test.bases import WorldTestBase
from test.general import gen_steps, setup_solo_multiworld as setup_base_solo_multiworld
Expand Down Expand Up @@ -365,7 +365,7 @@ def setup_solo_multiworld(test_options: Optional[Dict[Union[str, StardewValleyOp

if issubclass(option, VerifyKeys):
# Values should already be verified, but just in case...
option.verify_keys(value.value)
value.verify(StardewValleyWorld, "Tester", PlandoOptions.bosses)

setattr(args, name, {1: value})
multiworld.set_options(args)
Expand Down
Loading