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

SoE: use new AP API and naming and make APworld #2701

Merged
merged 13 commits into from
Jan 12, 2024

Conversation

black-sliver
Copy link
Member

@black-sliver black-sliver commented Jan 10, 2024

What is this fixing or adding?

  • get rid of deprecation warnings
    • new test base
    • new option api
  • get rid of logic mixin so we can haz apworld
    • should be slightly faster, but haven't benchmarked
  • use self.random instead of multiworld.random
  • rename files to new convention
  • rename tests to new convention
  • add a new test that was still in the queue
  • fix/add some typing

How was this tested?

  • unit tests
  • setup.py on py3.{8,9,10,11} on linux only
  • single player gens
  • mypy

worlds/soe/__init__.py Outdated Show resolved Hide resolved
worlds/soe/__init__.py Outdated Show resolved Hide resolved
@ScipioWright ScipioWright added the is: refactor/cleanup Improvements to code/output readability or organizization. label Jan 11, 2024
since i forgot to git add in the previous commit.
good thing we squash.
@black-sliver black-sliver merged commit e00b5a7 into ArchipelagoMW:main Jan 12, 2024
14 checks passed
@black-sliver black-sliver deleted the soe-ap044-api branch January 12, 2024 00:07
for field in fields(self):
option = getattr(self, field.name)
if isinstance(option, (EvermizerFlag, EvermizerFlags)):
flags += getattr(self, field.name).to_flag()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't notice before merge,
but wouldn't this be?
flags += option.to_flag()
so you don't have to getattr again?

Copy link
Member Author

@black-sliver black-sliver Jan 14, 2024

Choose a reason for hiding this comment

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

You are kind of right, I saw that after git add, but it needs a type ignore, or two separate instance() calls, or yet another type abstraction, so I decided to not bother at the moment.

Maybe if it isn't obvious why: Flag and Flags use a typing.Protocol for self and the two protocols are incompatible, so currently we'd need to specialize on each rather than use a tuple in isinstance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I didn't notice that those were mixins.

This is how I would do it:

        for field in fields(self):
            option = getattr(self, field.name)
            if isinstance(option, (EvermizerFlag, EvermizerFlags)):
                assert isinstance(option, Option)
                flags += option.to_flag()
        return flags

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. That seems like a super easy fix, but I don't understand it (and neither does pycharm 2023.2.3)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm guessing pycharm doesn't understand intersection types?

mypy and pyright see the intersection types:
option: <subclass of EvermizerFlag and Option> | <subclass of EvermizerFlags and Option>

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see. the assert creates "typings" that fulfil the individual protocol. Took me a while to see why this works.
And yeah, pycharm seems to think those are Option now, rather than a combined pseudo type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #2724

Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
* SoE: new file naming

also fixes test base deprecation

* SoE: use options_dataclass

* SoE: moar typing

* SoE: no more multiworld.random

* SoE: replace LogicMixin by SoEPlayerLogic object

* SoE: add test that rocket parts always exist

* SoE: Even moar typing

* SoE: can haz apworld now

* SoE: pep up test naming

* SoE: use self.options for trap chances

* SoE: remove unused import with outdated comment

* SoE: move flag and trap extraction to dataclass

as suggested by beauxq

* SoE: test trap option parsing and item generation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: refactor/cleanup Improvements to code/output readability or organizization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants