Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork93
Enforce local versions of objects#505
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
bd4ba0f todba0127Comparejaraco commentedSep 11, 2024
@abravalheri What do you think about this approach compared to#487? The nice thing about this approach is it provides simpler type expectations for downstream consumers while also not imposing any restrictions on providers. There's still a failing mypy check that I need to resolve, but the tests pass otherwise. Also, it's a little bit ugly that objects that can't be converted are cast instead, which is technically incorrect, but should provide a low-disruption signal to address those objects. I'll flag that in a code comment. |
importlib_metadata/_compat.py Outdated
| warnings.warn(f"Unrecognized distribution subclass{dist.__class__}") | ||
| returncast(importlib_metadata.Distribution,dist) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This block is perhaps the least respectable part of the proposed change. It means that any Distribution subclasses that aren't derived fromimportlib_metadata.Distribution orimportlib.metadata.PathDistribution (e.g. a custom subclass ofimportlib.metadata.Distribution) will get a warning and then will possibly not comply with the interface. It's no worse than the status quo, however, except that there's no indication that a type may not be complying with an interface. At least now, a warning will be issued.
The proper fix will be for providers to supplyimportlib_metadata.Distribution subclasses whenimportlib_metadata is present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you very much for having a look on this. The solution looks good for me, although I share the feeling that the simple cast may be a little fragile.
The proper fix will be for providers to supply importlib_metadata.Distribution subclasses when importlib_metadata is present.
Should this be officially communicated somehow to the developers? I believe that there is some opposition against using atry..except ifimportlib_metadata is not included explicitly as a package dependency. We can see an example of that feeling in the thread starting withpypa/pyproject-hooks#195 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure how the core devs would feel about adding this as a "tip" or recommendation in theimportlib.metadata docs...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should this be officially communicated somehow to the developers?
Yes, probably.
Not sure how the core devs would feel about adding this as a "tip" or recommendation in the
importlib.metadatadocs...
It should go wherever the docs are that provide guidance for providers. I'll figure out where that is and include it here or link it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Inpython/cpython#123976, I've filed an issue upstream to track the documentation, but as I think more about what the documentation says, I'm not confident in the approach. I'll continue the conversation in#486.
9504147 to4b4b091Comparejaraco commentedSep 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Tests are passing now on all but Python 3.8 and 3.9. They're failing on the Surely |
jaraco commentedSep 11, 2024
I think there are two issues going on: First, when running under pytest, some modules (namely plugins) get imported early, before assertion rewriting is turned on. That includes A second, related issue, is that the same issue happens during |
aa9c5ce to9fae83bComparejaraco commentedSep 11, 2024
I notice there is some small impact on performance: Nothing too scary. |
…rsions of PackagePath.
…sage and email.message.Message.
9fae83b to4a350a5Comparejaraco commentedSep 11, 2024
I confirmed that running |
Uh oh!
There was an error while loading.Please reload this page.
If a provider is returning objects from
importlib.metadata(Distribution,Message,PackagePath), replace them with local versions for predictable types for consumers and type checkers.Closes#486; supersedes#487