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

Zillion: use "new" settings api and cleaning #3903

Merged
merged 7 commits into from
Nov 29, 2024

Conversation

beauxq
Copy link
Collaborator

@beauxq beauxq commented Sep 7, 2024

What is this fixing or adding?

#3848 was the motivation for this,
but then I found a bunch of other stuff to clean up too.

  • a bunch of double quotes instead of single quotes
  • a bunch of @override decorators
  • removed a deprecated Logging.warn
  • fixed some builtin shadowing
  • fixed a few minor typing issues

How was this tested?

played a game

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Sep 7, 2024
@Exempt-Medic Exempt-Medic added the is: refactor/cleanup Improvements to code/output readability or organizization. label Sep 7, 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.

Have not tested, but code looks good.

The PR title is a bit confusing though, out of the 58 changed lines only 1 happens to match the title :P

If you don't see a reason to merge it now, I think I'd wait for a decision on 3848 - I believe we want to merge that right after 0.5.1 is out and have worlds fail.

worlds/zillion/patch.py Show resolved Hide resolved
@beauxq
Copy link
Collaborator Author

beauxq commented Sep 17, 2024

out of the 58 changed lines only 1 happens to match the title

It says "and cleaning".

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.

One comment, everything else looks good code-wise. Did not test anything since the non-typing changes are very few and I am assuming you tested gen an item links.

assert "players" in group
group_players = group["players"]
players_start_chars: List[Tuple[int, Chars]] = []
group_players = set(group["players"])
Copy link
Member

Choose a reason for hiding this comment

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

I has question: can a non-empty abstract set here be a frozenset?
If not, wouldn't an assert be better? You copy it and then assign it back, which seems fragile.
Alternatively group["players"] = group_players = set(group["players"]) would be an option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it did seem a little more fragile to separate those assignments.
I changed it to group["players"] = group_players = set(group["players"])

Even if the current code might not allow the frozenset with items, that seems more like an implementation detail that I shouldn't rely on.

I generated a few seeds with item links and looked at spoilers after this change.

@beauxq
Copy link
Collaborator Author

beauxq commented Nov 29, 2024

And I did test another playthrough after the more recent typing changes.
And I found that this is important: #4280
It might be better to merge that before this.

@black-sliver black-sliver merged commit 2fb59d3 into ArchipelagoMW:main Nov 29, 2024
16 checks passed
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. 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.

3 participants