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: Don't Modify Rules After set_rules #4566

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Exempt-Medic
Copy link
Member

What is this fixing or adding?

Adds a test that worlds don't modify Location.access_rule or Entrance.access_rule after set_rules. Also clarified this in the world API doc.

It's very barebones, there's probably a better way to format and perform the test.

Ocarina of Time and Pokemon Red and Blue are exempted from the test because they are exempt from the test_location_creation_steps test (they remove locations in generate_basic and pre_fill)

How was this tested?

The test, which TLoZ fails

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Jan 28, 2025
@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Jan 29, 2025
@NewSoupVi
Copy link
Member

NewSoupVi commented Jan 29, 2025

Couple of considerations.

  1. I think it's fine to modify rules after set_rules. I think a fully functional set should be present after set_rules, but I 100% think that changing them again afterwards has some valid use cases, like banning an item from a certain area for "fun" reasons by using an overly restrictive rule, but then switching back to the "actual" logic for spoiler log output.

  2. I think if entrances are allowed to stay unconnected until connect_entrances, they should be allowed to change rules all the way up until then as well. I can think of some scenarios where an entrance rando implementation using GER might want to change rules depending on which entrances were connected via a custom on_connect.

That said, I don't fully disagree with the need for a test or further documentation. I definitely think that setting your first ever rules in generate_basic is bad, for example. I wish I had a better counter proposal, but I'm coming up blank right now.

@Exempt-Medic Exempt-Medic added the waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. label Jan 29, 2025
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. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants