Skip to content

Conversation

@axxroytovu
Copy link
Contributor

@axxroytovu axxroytovu commented Jan 2, 2025

Still needs cleanup so I'm leaving this as draft for now.

What does this PR do?

  • Allows items and locations to individually have yaml_option definitions
  • Adds syntax to allow yaml_option to 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

@axxroytovu axxroytovu marked this pull request as ready for review January 19, 2025 05:12
is_location_enabled and is_item_enabled function input types were wrong and confusing
Split lambdas into two stages to prevent infinite recursion
Copy link
Contributor

@silasary silasary left a comment

Choose a reason for hiding this comment

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

Choices should also use =, not :

@axxroytovu
Copy link
Contributor Author

Easy enough change.

I kept : instead of = because it felt more consistent with other manual syntax that already uses : but I'm not married to it.

@axxroytovu axxroytovu requested a review from silasary January 19, 2025 16:53
silasary
silasary previously approved these changes Jan 19, 2025
Copy link
Contributor

@silasary silasary left a comment

Choose a reason for hiding this comment

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

If we're supporting >= and <=, we should probably also support !=?

@axxroytovu
Copy link
Contributor Author

It does already, but the format is "!option=value". Should be easy enough to also support "option!=value" as well.

@silasary
Copy link
Contributor

Yeah. Supporting both will lead to less support requests :D

nicopop added a commit that referenced this pull request Jan 25, 2025
Fixed hook type hints
Added `==` as valid option to match ManualForArchipelago#130 syntax
@FuzzyGamesOn
Copy link
Contributor

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.

@nicopop
Copy link
Contributor

nicopop commented Jan 20, 2026

Im currently working on fixing the issue this PR was waiting on and on the reason why this PR fails the Fuzz tests
here's a list of the changes I made so far:

  • Moved both call for resolve_yaml_option in is_*_enabled to _is_manualobject_enabled so there's not dup code and also respect category being enabled or not
  • added a None check to resolve_yaml_option's option getting so the type checker would be happy that in the event of a direct call to this func with a invalid option thing will only break in the way we want
  • Made resolve_yaml_option return only a bool like it did before this PR so any previous call to this helper function is not broken by this PR
    • this Fixed the Fuzz Test since starting_items evalutated None as false and thus skiped any block without a positive yaml_option result
    • This also mean any hooks made previously that called resolve_yaml_option could be broken if they did similar tests

If any of those changes are not something we want I can always revert them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants