Options: Always verify keys for VerifyKeys options (#3280)

* Options: Always verify keys for VerifyKeys options * fix PlandoTexts * use OptionError and give a slightly better error message for which option it is * add the player name to the error * don't create an unnecessary list --------- Co-authored-by: NewSoupVi <[email protected]>
ArchipelagoMW · Jul 31, 2024 · 53bc4ff · 53bc4ff
1 parent 91f7cf1
commit 53bc4ff
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 14 deletions.
diff --git a/Options.py b/Options.py
@@ -786,17 +786,22 @@ 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 OptionError(
+                    f"Found unexpected key {', '.join(extra)} in {getattr(self, 'display_name', self)}. "
+                    f"Allowed keys: {self._valid_keys}."
+                )
 
     def verify(self, world: typing.Type[World], player_name: str, plando_options: "PlandoOptions") -> None:
+        try:
+            self.verify_keys()
+        except OptionError as validation_error:
+            raise OptionError(f"Player {player_name} has invalid option keys:\n{validation_error}")
         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:
@@ -833,7 +838,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)}")
@@ -879,7 +883,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))
 
@@ -905,7 +908,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))
 
@@ -948,6 +950,19 @@ def verify(self, world: typing.Type[World], player_name: str, plando_options: "P
             self.value = []
             logging.warning(f"The plando texts module is turned off, "
                             f"so text for {player_name} will be ignored.")
+        else:
+            super().verify(world, player_name, plando_options)
+
+    def verify_keys(self) -> None:
+        if self.valid_keys:
+            data = set(text.at for text in self)
+            dataset = set(word.casefold() for word in data) if self.valid_keys_casefold else set(data)
+            extra = dataset - self._valid_keys
+            if extra:
+                raise OptionError(
+                    f"Invalid \"at\" placement {', '.join(extra)} in {getattr(self, 'display_name', self)}. "
+                    f"Allowed placements: {self._valid_keys}."
+                )
 
     @classmethod
     def from_any(cls, data: PlandoTextsFromAnyType) -> Self:
@@ -971,7 +986,6 @@ def from_any(cls, data: PlandoTextsFromAnyType) -> Self:
                         texts.append(text)
                 else:
                     raise Exception(f"Cannot create plando text from non-dictionary type, got {type(text)}")
-            cls.verify_keys([text.at for text in texts])
             return cls(texts)
         else:
             raise NotImplementedError(f"Cannot Convert from non-list, got {type(data)}")

diff --git a/worlds/stardew_valley/test/__init__.py b/worlds/stardew_valley/test/__init__.py
@@ -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
@@ -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)