Skip to content

Core: clarify error message when reading an APContainer #2887

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

Merged

Conversation

beauxq
Copy link
Collaborator

@beauxq beauxq commented Mar 3, 2024

What is this fixing or adding?

Since any game can override read_contents in APContainer (and might need to),
we could use a more clear error message if something goes wrong with that implementation.

A suggestion was made to do something with the version number that's defined in core.
But if the plan is to move games out of core, or have better support for games outside of core, then we can't use a patch version number in core to keep track of changes in game implementations.

The solution needs to be something that is available to games that have never been in core and never will be in core.

How was this tested?

I created a patch before this change: #2875
Then tested opening that patch with #2875 with and without these changes.

Before:

KeyError: "There is no item named 'gen_data.json' in the archive"

After:

KeyError: "There is no item named 'gen_data.json' in the archive"

The above exception was the direct cause of the following exception:

...

worlds.Files.InvalidDataError: There is no item named 'gen_data.json' in
the archive - This might be the incorrect world version for this file

@github-actions github-actions bot added waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. affects: core Issues/PRs that touch core and may need additional validation. labels Mar 3, 2024
self.read_contents(zf)
except Exception as e:
message = ""
if len(e.args):
Copy link
Member

Choose a reason for hiding this comment

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

does len really make sense here over just if e.args?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I dislike the python-ism of empty: false, non-empty:true
I've seen other people (in AP discord) say they dislike it also.
It doesn't match other languages, and can take people by surprise. So I prefer to avoid using it.

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.

This change is completely separate from why the version number exists and needs to be bumper. The version number is to make sure that the Patch program can understand what the patch is, not that the Patch program can actually patch the file.

This change for better error reporting is useful regardless of that.

@black-sliver black-sliver merged commit 4e31e51 into ArchipelagoMW:main Mar 3, 2024
@github-actions github-actions bot removed the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Mar 3, 2024
black-sliver pushed a commit that referenced this pull request Mar 4, 2024
I made this variable for more compatible and safer type narrowing, and then I didn't use if for the type narrowing.
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
I made this variable for more compatible and safer type narrowing, and then I didn't use if for the type narrowing.
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
I made this variable for more compatible and safer type narrowing, and then I didn't use if for the type narrowing.
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants