Yaml option expansion #126
Conversation
src/Options.py
Outdated
| for option_name in category_table[category].get("yaml_option", []): | ||
| for c in "><=:": # Range and Choice options must be defined using Options.json | ||
| if c in option_name: | ||
| continue |
There was a problem hiding this comment.
Can you make this check for option_name in manual_options, and throw an exception if they forgot to define it?
There was a problem hiding this comment.
Fixed in e5f27a8
Normalized ":" equality for all option types Removed imports from data and now reference multiworld/world data directly
is_location_enabled and is_item_enabled function input types were wrong and confusing Split lambdas into two stages to prevent infinite recursion
|
Easy enough change. I kept |
|
It does already, but the format is "!option=value". Should be easy enough to also support "option!=value" as well. |
|
Yeah. Supporting both will lead to less support requests :D |
|
|
||
| category_data = category_table.get(category_name, {}) | ||
| return resolve_yaml_option(multiworld, player, category_data) | ||
| category_data = multiworld.worlds[player].category_table.get(category_name, {}) |
There was a problem hiding this comment.
Was wondering why you referenced it this way and was reminded once again that the is_*_enabled hooks don't pass in world like all the other hooks, lol
(Just an observation. This way is fine, and the hooks can be standardized later.)
Fixed hook type hints Added `==` as valid option to match ManualForArchipelago#130 syntax
|
|
||
| try_resolve = resolve_yaml_option(multiworld, player, item) | ||
| if try_resolve is None: | ||
| return _is_manualobject_enabled(multiworld, player, item) | ||
| else: | ||
| return try_resolve |
There was a problem hiding this comment.
if I understand this part correctly if the item says its enabled via your new yaml_option then categories are not checked at all is that what you mean to do?
is that something we want?
idk but its something to think of
There was a problem hiding this comment.
actually there's a reason I originaly just put return _is_manualobject_enabled since the logic is the same for both loc and item maybe you could move your try_resolve call there so you dont have 2 copy of this
There was a problem hiding this comment.
if I understand this part correctly if the item says its enabled via your new yaml_option then categories are not checked at all is that what you mean to do?
Hmm. I could probably put an AND in there instead
|
At a glance, looks like this is just waiting on the resolve_yaml / manualobject_enabled adjustment. Skimmed over and looks good, will circle back to review more. |
4 participants
Still needs cleanup so I'm leaving this as draft for now.
What does this PR do?
yaml_optiondefinitionsyaml_optionto check range & choice options:"option>10","option<10","option=10"and"!option=10"all properly check against a range option"option:choice"properly checks against a choice option