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

fix: str of KeyError for <3.11 #2519

Merged
merged 3 commits into from
Jun 14, 2023
Merged

fix: str of KeyError for <3.11 #2519

merged 3 commits into from
Jun 14, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Jun 9, 2023

This fixes #2513

@agoose77 agoose77 requested a review from jpivarski June 9, 2023 14:38
@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #2519 (3483e2b) into main (60114c5) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

❗ Current head 3483e2b differs from pull request most recent head e59cb86. Consider uploading reports for the commit e59cb86 to get more accurate results

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_errors.py 80.27% <66.66%> (-0.59%) ⬇️

... and 3 files with indirect coverage changes

@agoose77 agoose77 temporarily deployed to docs-preview June 9, 2023 14:50 — with GitHub Actions Inactive
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

I'm uncomfortable about our class pretending to be the builtins.KeyError. It can be named KeyError, but let's make it distinguishable by __module__, so that when someone is digging into a bug, we're not making the solution harder for them.

(I'm assuming that experts who are trying to resolve bugs would know that classes can have the same name in different modules.)

Comment on lines 40 to 43
# Pretend to be builtins!
AwkwardKeyError.__name__ = "KeyError"
AwkwardKeyError.__qualname__ = "KeyError"
AwkwardKeyError.__module__ = "builtins"
Copy link
Member

Choose a reason for hiding this comment

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

I'm wary of this. It could lead to someone being very confused that this type is not KeyError for some reason.

I was thinking that we would name the class ak._errors.KeyError, so it displays its name as KeyError, but it's distinguishable by its __module__. Setting __module__ = "builtins" sounds like we're setting up for a disaster.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's fair. In general, it's not good practice to use 'is' like this, but I think you make a reasonable argument. Let's rework this in line with making our error distinguishable from the built-in.

@agoose77 agoose77 temporarily deployed to docs-preview June 10, 2023 14:35 — with GitHub Actions Inactive
@agoose77 agoose77 requested a review from jpivarski June 14, 2023 17:51
@jpivarski
Copy link
Member

I made a counter-edit to indicate what I meant. If we're saying its name is KeyError, then we should let its name be KeyError; module encapsulation is for making these sorts of same-name-but-distinct references.

It means that we can never use unqualified KeyError in the text of this file—it always has to be builtins.KeyError—but that's normal. Uproot's reading.py has to do that with the function named open, and ak_zip.py has to do that with the function named zip.

Comment on lines +96 to +97
elif issubclass(cls, builtins.KeyError):
new_exception = KeyError(self.format_exception(exception))
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 prefer not to shadow the KeyError builtin, as it's not something one usually overrides in a global namespace. Our AwkwardKeyError isn't something I anticipated even L3 awkward modules importing, so changing __name__ didn't both me. With some time having elapsed, I have changed my mind on that, so I'm now in favour of this change.

@agoose77 agoose77 enabled auto-merge (squash) June 14, 2023 18:32
@jpivarski jpivarski temporarily deployed to docs-preview June 14, 2023 18:43 — with GitHub Actions Inactive
@agoose77 agoose77 merged commit f81d2bf into main Jun 14, 2023
34 checks passed
@agoose77 agoose77 deleted the agoose77/fix-str-keyerror branch June 14, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error formatting is broken (an error in the error handling)
2 participants