[Core] Add World.status and improved mechanisms for world hiding/disabling #3775

BadMagic100 · 2024-08-11T19:36:57Z

What is this fixing or adding?

Adds improvements to hiding/disabling worlds via a Status enum for worlds, rather than just using world.hidden

The goals of this PR are as follows:

Add a way to disable a world without using worlds_disabled. This allows any work on these worlds to be done more seamlessly without risking conflicts when worlds are enabled/disabled
Add a way to disable tests for and "soft disable" a world, causing it to be hidden and warn users when the world is used in generation. This would allow the world to be disabled in some way while still keeping it available for website users as long as it is not strictly broken
Document the process for soft disabling vs hard disabling a world
Use the new disabling mechanism for our 1 disabled world

Having the webhost surface the warning to website users is currently being considered out of scope for this change. In the future we should likely consider this before actually soft-disabling worlds.

If there is anything we should be doing in setup.py around worlds_disabled (there is a reference to it there) I'm not sure what.

How was this tested?

Tested that ori does not load/run tests and that hidden worlds are still hidden to the website. Ran the entire test suite in both pycharm and pytest with xdist.

agilbert1412

I like what I'm seeing so far, but since the PR isn't complete, I'm not approving yet. Will look again later, don't hesitate to ping me if/when it's updated

BadMagic100 · 2024-09-12T03:15:43Z

Logging some early feedback from black sliver in discord

He doesn't like the name of the enum/field (minor)
Apparently pytest discovery will not play nice with these changes as written at this point in time

Pytest compatibility, new "warning" level

…get_testable_world_types for better future extensibility.

BadMagic100 · 2024-09-15T20:45:24Z

@agilbert1412 this is ready for review

agilbert1412

Only one very small comment. Other than that, I like the way this has been implemented. A lot of the changes are the same thing over and over, which seems to suggest maybe some code duplication could be improved in these files, but definitely out of scope for this PR. Just keep an eye out, if you do make changes, that you might need to do it many times.

agilbert1412 · 2024-09-25T00:58:43Z

worlds/archipidle/__init__.py

@@ -33,7 +33,7 @@ class ArchipIDLEWorld(World):
    """
    game = "ArchipIDLE"
    topology_present = False
-    hidden = (datetime.now().month != 4)  # ArchipIDLE is only visible during April
+    status = Status.enabled if (datetime.now().month == 4) else Status.hidden_enabled


I would still keep the comment explaining the April thing

Suggested change

status = Status.enabled if (datetime.now().month == 4) else Status.hidden_enabled

status = Status.enabled if (datetime.now().month == 4) else Status.hidden_enabled # Only visible during April

BadMagic100 · 2024-09-25T03:56:04Z

Yeah, all the code duplication is there already and painful to adjust en masse. Not really sure if there would be a better way to deal with it, as it's all "iterate and subtest each testable world" and at the very least that code duplication will always be there in every relevant test even if we did something like introduce a decorator that does it automatically (which is actually not a half-bad idea tbh but is definitely out of scope and would not prevent a similar problem in the future)

qwint

reviewed wording changes in docs files,
tested generating with a broken but soft_disabled world to see the warning
tested pytest with a broken world using all enum values, seeing the failures on enabled and hidden, and not seeing test failures on the two disabled versions
retested with unittest to see the same results

Didn't extensively review the code changes but did skim for any glaring issues
Didn't test hidden feature on webhost (although that seems mostly untouched, just set differently)

The one big issue I saw was:
Disabled worlds don't get considered in the Exception: No world found to handle game . . . error message, there should probably be a unique error message if a world in generation is disabled noting that.

qwint · 2024-10-04T17:15:31Z

docs/world maintainer.md

+
+In the case that an unmaintained world is completely broken, the world can be marked as disabled. For each disabled 
+world, a README file can be found detailing when the world was disabled and the reasons that it was disabled. In order 
+to be considered for reactivation, these concerns should be handled at a bare minimum. However, each world may have\


Suggested change

to be considered for reactivation, these concerns should be handled at a bare minimum. However, each world may have\

to be considered for reactivation, these concerns should be handled at a bare minimum. However, each world may have

looked like a typo, and on the resolved markdown just puts a random newline in the paragraph

qwint · 2024-10-04T17:27:59Z

BaseClasses.py

@@ -228,6 +228,10 @@ def set_options(self, args: Namespace) -> None:

        for player in self.player_ids:
            world_type = AutoWorld.AutoWorldRegister.world_types[self.game[player]]
+            if world_type.status == AutoWorld.Status.soft_disabled:
+                logging.warning(f"Player {player} is playing {world_type.game}, "


this may be confusing as slot number instead of slot name
since it will trigger for every slot playing that game i'm not that worried about it, but it could make tracking down people's yamls in a submitted multi harder

qwint · 2024-10-04T18:05:57Z

Also, the pytest handling seems to run before even pytest -h which is annoying but not end of the world

Exempt-Medic · 2024-11-20T20:28:51Z

docs/world maintainer.md

+When a world becomes unmaintained, it can stay enabled as long as it is not breaking. At the discretion of the core
+maintainers, unmaintained worlds may also have their tests disabled at which point they will be marked as hidden
+and a warning will be provided when players try to generate with that world. This provides the core maintainers
+a mechanism to "soft disable" a world in order to allow core changes/tests to be merge which would not strictly break 


Suggested change

a mechanism to "soft disable" a world in order to allow core changes/tests to be merge which would not strictly break

a mechanism to "soft disable" a world in order to allow core changes/tests to be merged which would not strictly break

Exempt-Medic · 2024-11-21T00:58:25Z

BaseClasses.py

@@ -228,6 +228,10 @@ def set_options(self, args: Namespace) -> None:

        for player in self.player_ids:
            world_type = AutoWorld.AutoWorldRegister.world_types[self.game[player]]
+            if world_type.status == AutoWorld.Status.soft_disabled:
+                logging.warning(f"Player {player} is playing {world_type.game}, "
+                                f"which is currently considered to be unstable."


Missing space

Suggested change

f"which is currently considered to be unstable."

f"which is currently considered to be unstable. "

Exempt-Medic · 2024-11-28T14:53:47Z

When using this I got "OriBlindForest Assigned options through option_definitions which is now deprecated. Please use options_dataclass instead." which seems unintended

BadMagic100 · 2025-01-15T03:21:34Z

That does seem unintentional, I guess the classvar assignments would still get to happen for disabled worlds in this scheme (don't mind the closing and opening my mobile keyboard was too close to close with comment)

BadMagic100 · 2025-01-15T03:23:56Z

Also, the pytest handling seems to run before even pytest -h which is annoying but not end of the world

Unfortunately the pytest handling is not really my area of expertise, @Silvris any clue if this is avoidable?

BadMagic100 · 2025-01-27T20:14:45Z

When using this I got "OriBlindForest Assigned options through option_definitions which is now deprecated. Please use options_dataclass instead." which seems unintended

After some research I believe the fix here is to just check the attr early and return None, short circuiting past any other logic which may or may not happen including during super().__new__

BadMagic100 · 2025-01-28T18:38:06Z

Closing this. There is general alignment this is not the right solution to the problem.

Add World.visibility; update hidden worlds, remove worlds_disabled

346ed00

github-actions bot added the affects: core Issues/PRs that touch core and may need additional validation. label Aug 11, 2024

Remove reference to worlds_disabled in labeler.yml

c311aba

agilbert1412 self-requested a review August 11, 2024 20:25

agilbert1412 reviewed Aug 11, 2024

View reviewed changes

Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Aug 11, 2024

Silvris and others added 8 commits September 11, 2024 23:25

pytest compat, warning level

a7884c4

pep

58f1232

update core tests

234085c

3.8 compat

7581b90

Merge pull request #22 from Silvris/world_visibility_pytest

ca25d09

Pytest compatibility, new "warning" level

Merge branch 'main' into world_visibility

c8f6e28

Rename world.visibility to world.status. Introduce AutoWorldRegister.…

636474a

…get_testable_world_types for better future extensibility.

Improve conftest logging

d94f0d6

BadMagic100 changed the title ~~[Core] Add World.visibility and improved mechanisms for world hiding/disabling~~ [Core] Add World.status and improved mechanisms for world hiding/disabling Sep 15, 2024

github-actions bot added the affects: webhost Issues/PRs that touch webhost and may need additional validation. label Sep 15, 2024

BadMagic100 added 2 commits September 15, 2024 13:36

Revert incorrect webhost changes

52c7851

Remove unnecessary if check

10285c1

BadMagic100 removed the affects: webhost Issues/PRs that touch webhost and may need additional validation. label Sep 15, 2024

BadMagic100 marked this pull request as ready for review September 15, 2024 20:44

BadMagic100 requested a review from LegendaryLinux as a code owner September 15, 2024 20:44

BadMagic100 added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Sep 15, 2024

agilbert1412 approved these changes Sep 25, 2024

View reviewed changes

qwint suggested changes Oct 4, 2024

View reviewed changes

Exempt-Medic added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Nov 20, 2024

Exempt-Medic reviewed Nov 20, 2024

View reviewed changes

Exempt-Medic reviewed Nov 21, 2024

View reviewed changes

BadMagic100 closed this Jan 15, 2025

BadMagic100 reopened this Jan 15, 2025

BadMagic100 closed this Jan 27, 2025

BadMagic100 reopened this Jan 27, 2025

BadMagic100 closed this Jan 28, 2025

BadMagic100 deleted the world_visibility branch January 28, 2025 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Add World.status and improved mechanisms for world hiding/disabling #3775

[Core] Add World.status and improved mechanisms for world hiding/disabling #3775

BadMagic100 commented Aug 11, 2024 •

edited

Loading

agilbert1412 left a comment

BadMagic100 commented Sep 12, 2024

BadMagic100 commented Sep 15, 2024

agilbert1412 left a comment

agilbert1412 Sep 25, 2024

Exempt-Medic Nov 20, 2024

BadMagic100 commented Sep 25, 2024 •

edited

Loading

qwint left a comment

qwint Oct 4, 2024

qwint Oct 4, 2024

qwint commented Oct 4, 2024

Exempt-Medic Nov 20, 2024

Exempt-Medic Nov 21, 2024

Exempt-Medic commented Nov 28, 2024 •

edited

Loading

BadMagic100 commented Jan 15, 2025 •

edited

Loading

BadMagic100 commented Jan 15, 2025

BadMagic100 commented Jan 27, 2025

BadMagic100 commented Jan 28, 2025

	status = Status.enabled if (datetime.now().month == 4) else Status.hidden_enabled
	status = Status.enabled if (datetime.now().month == 4) else Status.hidden_enabled # Only visible during April

	to be considered for reactivation, these concerns should be handled at a bare minimum. However, each world may have\
	to be considered for reactivation, these concerns should be handled at a bare minimum. However, each world may have

	a mechanism to "soft disable" a world in order to allow core changes/tests to be merge which would not strictly break
	a mechanism to "soft disable" a world in order to allow core changes/tests to be merged which would not strictly break

	f"which is currently considered to be unstable."
	f"which is currently considered to be unstable. "

[Core] Add World.status and improved mechanisms for world hiding/disabling #3775

[Core] Add World.status and improved mechanisms for world hiding/disabling #3775

Conversation

BadMagic100 commented Aug 11, 2024 • edited Loading

What is this fixing or adding?

How was this tested?

agilbert1412 left a comment

Choose a reason for hiding this comment

BadMagic100 commented Sep 12, 2024

BadMagic100 commented Sep 15, 2024

agilbert1412 left a comment

Choose a reason for hiding this comment

agilbert1412 Sep 25, 2024

Choose a reason for hiding this comment

Exempt-Medic Nov 20, 2024

Choose a reason for hiding this comment

BadMagic100 commented Sep 25, 2024 • edited Loading

qwint left a comment

Choose a reason for hiding this comment

qwint Oct 4, 2024

Choose a reason for hiding this comment

qwint Oct 4, 2024

Choose a reason for hiding this comment

qwint commented Oct 4, 2024

Exempt-Medic Nov 20, 2024

Choose a reason for hiding this comment

Exempt-Medic Nov 21, 2024

Choose a reason for hiding this comment

Exempt-Medic commented Nov 28, 2024 • edited Loading

BadMagic100 commented Jan 15, 2025 • edited Loading

BadMagic100 commented Jan 15, 2025

BadMagic100 commented Jan 27, 2025

BadMagic100 commented Jan 28, 2025

BadMagic100 commented Aug 11, 2024 •

edited

Loading

BadMagic100 commented Sep 25, 2024 •

edited

Loading

Exempt-Medic commented Nov 28, 2024 •

edited

Loading

BadMagic100 commented Jan 15, 2025 •

edited

Loading