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

Core: log warning for unknown options #1385

Merged
merged 15 commits into from
May 10, 2024

Conversation

alwaysintreble
Copy link
Collaborator

Please format your title with what portion of the project this pull request is
targeting and what it's changing.

ex. "MyGame4: implement new game" or "Docs: add new guide for customizing MyGame3"

What is this fixing or adding?

title

How was this tested?

rolled seeds with valid and invalid option names

If this makes graphical changes, please attach screenshots.

Screenshot_47

@KonoTyran
Copy link
Contributor

So this will break things like mercenary mode yamls for alttp as they use and set "invalid" keys in the yaml.

@alwaysintreble
Copy link
Collaborator Author

It marks trigger option names as valid.

Generate.py Outdated Show resolved Hide resolved
@alwaysintreble
Copy link
Collaborator Author

Ran this against the weights file for the big async and turned out I didn't notice that roll_triggers was called 2 separate times and would give different results based on when it was called and would find invalid keys for other games even if that game wasn't being played. Fixed this by only doing the validation step after all triggers are finished resolving and having a global set of valid_keys to check against for both trigger resolutions now. Found a bunch of typos in the big async weights file with it and it succeeded with some custom names in it.

@Jarno458
Copy link
Collaborator

Jarno458 commented Feb 1, 2023

I am not sure if i like this, like for timespinner i can basicly use the same yaml for (that includes beta options) that works on both normal and beta. those new beta options simply wont have any effect but wont break anything either

@alwaysintreble
Copy link
Collaborator Author

The point is to fail for options that were removed. While i don't think people should be removing options they're usually modified in some way so this solves that. When i was testing this with the async weights file there were about 3 options being rolled that no longer exist that wouldn't have been found otherwise. Supporting beta content is out of scope of this especially when it's just a minor convenience.

@Jarno458
Copy link
Collaborator

Jarno458 commented Feb 1, 2023

but does it really matter? if the option is removed well it wont do a thing right? I don't think this warrants to error out
I am seriously concerned that this will be more of a annoyance every time it fails because of this over the value it adds

@Berserker66
Copy link
Member

My proposal was to introduce hidden options, and therefore give maintainers the ability to make a hidden option that errors out when set, if they want to loudly deprecate an option. I too worry this will lead to more annoyance than it's worth, personally.

@alwaysintreble
Copy link
Collaborator Author

I still don't agree but it now just logs a warning for unknown options instead of crashing.

@ThePhar ThePhar 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 May 31, 2023
@alwaysintreble alwaysintreble changed the title Core: throw an error for unknown options Core: log warning for unknown options Oct 3, 2023
@ScipioWright ScipioWright added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Feb 27, 2024
@Exempt-Medic Exempt-Medic added waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. and removed waiting-on: author Issue/PR is waiting for feedback or changes from its author. labels Apr 26, 2024
Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

Briefly looked at the code, merged into main and did functional testing, trying various methods of using triggers and options. It gave warnings only when it was supposed to. One small wording suggestion, otherwise it seems to work just fine

Generate.py Outdated Show resolved Hide resolved
Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
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.

Since this is now a warning, I think I'm ok merging this. If there are issues because of a case we didn't consider, we can work them out and they at least won't break anything.

For the record, this really reaffirms my belief that the Webhost should output warnings somehow.

@NewSoupVi NewSoupVi merged commit af83050 into ArchipelagoMW:main May 10, 2024
16 checks passed
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
* throw an error for unknown options

* move the error to the end of trigger resolution and make trigger names valid

* add bad hardcoded stuff for LTTP

* use itertools.chain instead of a ChainMap

* remove accidental unused import

* make the check after both trigger resolutions so no valid keys are missed, and only check relevant game.

* log a warning instead of crashing

* delete options from the weights once it gets registered for cleaner erroring

* grammar hard

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>

---------

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
* throw an error for unknown options

* move the error to the end of trigger resolution and make trigger names valid

* add bad hardcoded stuff for LTTP

* use itertools.chain instead of a ChainMap

* remove accidental unused import

* make the check after both trigger resolutions so no valid keys are missed, and only check relevant game.

* log a warning instead of crashing

* delete options from the weights once it gets registered for cleaner erroring

* grammar hard

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>

---------

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
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: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants