| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
| warnings.warn(f"Unrecognized distribution subclass {dist.__class__}") | ||
| return cast(importlib_metadata.Distribution, dist) |
There was a problem hiding this comment.
This block is perhaps the least respectable part of the proposed change. It means that any Distribution subclasses that aren't derived from importlib_metadata.Distribution or importlib.metadata.PathDistribution (e.g. a custom subclass of importlib.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 supply importlib_metadata.Distribution subclasses when importlib_metadata is present.
Sorry, something went wrong.
There was a problem hiding this comment.
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 a try..except if importlib_metadata is not included explicitly as a package dependency. We can see an example of that feeling in the thread starting with pypa/pyproject-hooks#195 (comment).
Sorry, something went wrong.
There was a problem hiding this comment.
Not sure how the core devs would feel about adding this as a "tip" or recommendation in the importlib.metadata docs...
Sorry, something went wrong.
There was a problem hiding this comment.
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.metadata docs...
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.
Sorry, something went wrong.
There was a problem hiding this comment.
In python/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.
Sorry, something went wrong.
|
Tests are passing now on all but Python 3.8 and 3.9. They're failing on the project unit (pytest-checkdocs). It seems that the warning functionality is somehow getting triggered for pytest-checkdocs even though the class is of the intended type: .::project
tests/data/sources/example2::project
/home/runner/work/importlib_metadata/importlib_metadata/importlib_metadata/_compat.py:80: UserWarning: Unrecognized distribution subclass <class 'importlib_metadata.PathDistribution'>
warnings.warn(f"Unrecognized distribution subclass {dist.__class__}")
Surely importlib_metadata.PathDistribution is an instance of importlib_metadata.Distribution. I need to figure out what's causing that not to be the case. |
Sorry, something went wrong.
|
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 pytest_checkdocs, which imports jaraco.packaging which imports build, which imports importlib_metadata. When later importlib_metadata is re-imported with the assert-rewrite support, a new copy is imported such that the types aren't comparable using isinstance: > /Users/jaraco/code/python/importlib_metadata/importlib_metadata/_compat.py(82)localize_dist() -> return cast(importlib_metadata.Distribution, dist) (Pdb) importlib_metadata <module 'importlib_metadata' from '/Users/jaraco/code/python/importlib_metadata/importlib_metadata/__init__.py'> (Pdb) build._compat.importlib.metadata <module 'importlib_metadata' from '/Users/jaraco/code/python/importlib_metadata/importlib_metadata/__init__.py'> (Pdb) importlib_metadata is build._compat.importlib.metadata False A second, related issue, is that the same issue happens during localize_metadata. Because the isinstance check fails, the importlib_metadata._adapters.Message object gets re-constructed from an existing message (of the same class), causing the redent function to be invoked twice on the Description, mangling the reStructuredText (by adding ' '*8 at the beginning). |
Sorry, something went wrong.
|
I notice there is some small impact on performance: exercises.py:cached distribution: 0:00:00.000302 (+0:00:00.000007, 2%) exercises.py:discovery: 0:00:00.000300 (+0:00:00.000003, 1%) exercises.py:entry_points(): 0:00:00.002260 (+0:00:00.000040, 2%) exercises.py:entrypoint_regexp_perf: 0:00:00.000059 (+0:00:00.000001, 2%) exercises.py:uncached distribution: 0:00:00.000452 (+0:00:00.000002, 0%) Nothing too scary. |
Sorry, something went wrong.
…rsions of PackagePath.
…sage and email.message.Message.
…data, is present.
I confirmed that running pytest --assert plain against 1a30e01 (prior to the workarounds), the tests pass, confirming that the issue was indeed isolated to the pytest assert rewrite importer. |
Sorry, something went wrong.
# Conflicts: # importlib_metadata/__init__.py # tests/compat/test_py39_compat.py
…one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Importing importlib.metadata and _adapters eagerly regressed import time by 17 msec (366%), largely defeating python/cpython#109829. Defer them to the paths that need them, leaving a 4% cost for the module itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Asserting that importlib.metadata satisfies the protocols is a concern of the backport alone, so it moves to the compat package, where it can also account for Distribution.origin arriving in Python 3.13. What remains asserts that the objects this package supplies conform. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers the scenario reported in #486, restoring the coverage lost when the Python 3.9 compatibility tests were removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
A suggestion that came out of documenting this downstream (python/cpython#160344, for python/cpython#123976): consider warning on conformance rather than on class identity. Today localize.dist warns for any Distribution subclass whose module isn't importlib_metadata: if dist.__class__.__module__ != 'importlib_metadata':
warnings.warn(f"Unrecognized distribution subclass {dist.__class__}")Now that this PR adds IDistribution, that guard could ask whether the object actually satisfies the interface instead. The difference matters for the downstream guidance. As it stands, a provider that implements IDistribution completely still warns, and the person who sees the warning is not the person who can act on it. The custom-provider example in the CPython docs is exactly this case: a DatabaseDistribution that subclasses importlib.metadata.Distribution, implements read_text and locate_file, and inherits the rest. It works correctly under the backport and it warns. So the stdlib text has to explain a warning that providers cannot clear except by depending on this package, which is the coupling #486 set out to avoid. With a conformance check, the guidance collapses to "implement the whole interface", and the warning means something is genuinely missing. One implementation note, since it is not obvious: isinstance(dist, IDistribution) is not the way to spell it. With requires-python = ">=3.11" and IDistribution carrying property members, isinstance against a runtime-checkable protocol evaluates those properties on 3.11, so the check would read METADATA for every distribution just to decide whether to warn, and a property that raises propagates out of isinstance. Measured on a protocol with metadata and version properties:
Checking the type avoids that on every supported version, and can name what is missing: _REQUIRED = (
'read_text', 'locate_file', 'metadata', 'name',
'version', 'entry_points', 'files', 'requires',
)
if missing := [name for name in _REQUIRED if not hasattr(type(dist), name)]:
warnings.warn(
f"Distribution subclass {dist.__class__} is missing {', '.join(missing)}"
)(issubclass is not available here: protocols with non-method members reject it.) This would also subsume the __module__ check that currently works around pytest replacing importlib_metadata, since a replaced class still conforms. Entirely possible the identity check is deliberate and I am missing a case it protects. The stdlib text works either way; it is just longer, and asks providers to live with a warning rather than to fix something. |
Sorry, something went wrong.
The stdlib only grew Distribution.origin in Python 3.13, so declaring it makes the protocol describe something importlib.metadata does not supply on 3.11 or 3.12, which in turn forced the conformance test to skip there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Through Python 3.11, isinstance() against a runtime-checkable protocol calls hasattr() on the instance, so asking whether a distribution conforms would read its METADATA, and a property that raises would escape the question. Python 3.12 resolved that in python/cpython#102433, so defer to isinstance there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A provider that implements the whole interface was warned about anyway for deriving from importlib.metadata, and the person who saw the warning was not the person who could act on it; clearing it meant depending on this package, the coupling #486 set out to avoid. Ask instead what the interface requires and the class omits, and name it. This subsumes the __module__ check that worked around pytest replacing importlib_metadata, since a replaced class still conforms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A provider cannot implement a member its importlib.metadata has no version of, so faulting it for the absence would reintroduce the warning nobody can clear. Collect those members in compat/py312, to be deleted along with support for the Python that needs them. The decision splits at 3.13 rather than 3.12 as a result, so the type-only check moves here; plain isinstance still answers it wherever nothing is exempt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Consumers should be able to rely on it, and the exemption now keeps a provider on Python 3.11 or 3.12, whose stdlib has no origin to inherit, from being warned about its absence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Assume the shared protocols from python/importlib_metadata#505 are presented by this module as well: document IDistribution and IPackagePath, correct PackageMetadata, which is already a protocol here rather than an implementation of the backport's, and write the caveats around the protocols as the interface both libraries satisfy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| Back | FazBrowse Home | New Git URL |
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