python / importlib_metadata

Library to access metadata for Python packages
https://importlib-metadata.readthedocs.io
Apache License 2.0
128 stars 82 forks source link

Enforce local versions of objects #505

Open jaraco opened 2 months ago

jaraco commented 2 months ago

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

jaraco commented 2 months ago

@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.

jaraco commented 2 months ago

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.

jaraco commented 2 months ago

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).

jaraco commented 2 months ago

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.

jaraco commented 2 months ago

I think there are two issues going on:

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.