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

Tests: add test that option classes aren't reused #3530

Merged
merged 3 commits into from
Oct 30, 2024

Conversation

alwaysintreble
Copy link
Collaborator

What is this fixing or adding?

Prevents worlds from reusing the exact same option class in their available options. This becomes problematic when trying to use Option Groups since all the grouping happens via the actual class and not the dataclass's attributes. Namely #3393

How was this tested?

Ran tests a few times, added a duplicate option and removed it ensuring it behaved how I expected.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Jun 14, 2024
@Silvris
Copy link
Collaborator

Silvris commented Jun 14, 2024

Should this ignore Removed? Unsure if they go into Game options by default like other options, but it is the main use I can see of a duplicate class.

@alwaysintreble
Copy link
Collaborator Author

it ignores Visibility.none entirely

@alwaysintreble alwaysintreble added is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. labels Jun 14, 2024
@alwaysintreble alwaysintreble requested a review from ThePhar June 14, 2024 03:30
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.

I agree with this test, the code looks good, and only one option from world fails it

@Exempt-Medic
Copy link
Member

This should work now btw

@Exempt-Medic Exempt-Medic removed the waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. label Sep 13, 2024
Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

I agree with this because the display name for the option is per class, making it bad UX to reuse the class.

@black-sliver black-sliver merged commit aaf25f8 into ArchipelagoMW:main Oct 30, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants