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.

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

Rules: Verify the default values of Options. #2403

Merged
merged 4 commits into from
May 2, 2024

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Oct 30, 2023

What is this fixing or adding?

Since Option.verify() can handle normalization of option names, this allows options to define defaults which rely on that normalization. For example, it allows a world to exclude certain locations by default.

This also makes it easier to catch errors if a world author accidentally sets an invalid default.

How was this tested?

I set up a world to exclude a name group by default, ran a generation with default options, and verified that all locations in that group were excluded.

Since `Option.verify()` can handle normalization of option names, this allows options  to define defaults which rely on that normalization. For example, it allows a world to exclude certain locations by default.

This also makes it easier to catch errors if a world author accidentally sets an invalid default.
@ScootyPuffJr1 ScootyPuffJr1 added is: enhancement Issues requesting new features or pull requests implementing new features. affects: core Issues/PRs that touch core and may need additional validation. labels Oct 30, 2023
Generate.py Outdated Show resolved Hide resolved
Co-authored-by: Doug Hoskisson <[email protected]>
@PoryGone PoryGone added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Mar 6, 2024
Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

Change makes sense, I tested it by excluding the "Tutorial (Outside)" group by default in The Witness, which correctly evaluated to the corresponding locations.

Comment on lines 436 to +437
else:
setattr(ret, option_key, option.from_any(option.default)) # call the from_any here to support default "random"
player_option.verify(AutoWorldRegister.world_types[ret.game], ret.name, plando_options)
Copy link
Member

Choose a reason for hiding this comment

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

I did not know try -> except -> else existed. Reading up on it, it seems kind of not necessary in this case, as the except block re-raises the Exception anyway, which exits the function, meaning there is no scenario in which "you need to prevent something running after the except". But maybe I'm missing nuance, and hey, it's not wrong or anything, it just took me by surprise. Also it mirrors the old code so I guess it's fine

@Exempt-Medic Exempt-Medic added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Apr 1, 2024
@nex3 nex3 changed the title Ruls: Verify the default values of Options. Rules: Verify the default values of Options. Apr 1, 2024
@Exempt-Medic Exempt-Medic mentioned this pull request Apr 12, 2024
@NewSoupVi NewSoupVi merged commit 9d478ba into ArchipelagoMW:main May 2, 2024
15 checks passed
@nex3 nex3 deleted the option-verify-default branch May 2, 2024 20:22
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
* Verify the default values of `Option`s.

Since `Option.verify()` can handle normalization of option names, this allows options  to define defaults which rely on that normalization. For example, it allows a world to exclude certain locations by default.

This also makes it easier to catch errors if a world author accidentally sets an invalid default.

* Update Generate.py

Co-authored-by: Doug Hoskisson <[email protected]>

---------

Co-authored-by: Doug Hoskisson <[email protected]>
Co-authored-by: NewSoupVi <[email protected]>
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
* Verify the default values of `Option`s.

Since `Option.verify()` can handle normalization of option names, this allows options  to define defaults which rely on that normalization. For example, it allows a world to exclude certain locations by default.

This also makes it easier to catch errors if a world author accidentally sets an invalid default.

* Update Generate.py

Co-authored-by: Doug Hoskisson <[email protected]>

---------

Co-authored-by: Doug Hoskisson <[email protected]>
Co-authored-by: NewSoupVi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants