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: Prevent worlds from using LogicMixin incorrectly (having class variables without an init_mixin) #3974

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

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented Sep 21, 2024

There's a world that ran into some issues because it defined its custom LogicMixin variables at the class level.

This caused "instance bleed" when new CollectionState objects were created.

I don't think there is ever a reason to have a non-function class variable on LogicMixin without also having init_mixin, so this asserts that this is the case.

Tested:
Doesn't fail any current worlds
Correctly fails the world in question

Also, not gonna call out that world specifically, because it was literally my fault for explaining it to them wrong :D

There's a world that ran into some issues because it defined its custom LogicMixin variables at the class level.

This caused "instance bleed" when new CollectionState objects were created.

I don't think there is ever a reason to have a non-function class variable on LogicMixin without also having `init_mixin`, so this asserts that this is the case.

Tested:
Doesn't fail any current worlds
Correctly fails the world in question

Also, not gonna call out that world because it was literally my fault for explaining it to them wrong :D
@github-actions github-actions bot added the affects: core Issues/PRs that touch core and may need additional validation. label Sep 21, 2024
@NewSoupVi NewSoupVi added the is: refactor/cleanup Improvements to code/output readability or organizization. label Sep 21, 2024
@NewSoupVi NewSoupVi changed the title Core: Prevent people from using LogicMixin incorrectly Core: Prevent worlds from using LogicMixin incorrectly (having class variables without an init_mixin) Sep 21, 2024
@NewSoupVi NewSoupVi added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Sep 21, 2024
@NewSoupVi
Copy link
Member Author

NewSoupVi commented Sep 21, 2024

There is a part of this where worlds might be putting "static data" onto their LogicMixin class, which might lead to "false positives", but also, I don't really see a use case for LogicMixin without init_mixin in general, so actually I would say that this implementation still has false negatives - But making it even stricter (requiring init_mixin always) would require me touching a bunch of worlds in non trivial ways

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: refactor/cleanup Improvements to code/output readability or organizization. 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.

1 participant