Skip to content

Comments

Feature implementation from commits 57b8aa8..3f78dc1#4

Open
yashuatla wants to merge 1601 commits intofeature-base-4from
feature-head-4
Open

Feature implementation from commits 57b8aa8..3f78dc1#4
yashuatla wants to merge 1601 commits intofeature-base-4from
feature-head-4

Conversation

@yashuatla
Copy link
Owner

@yashuatla yashuatla commented Jun 22, 2025

PR Summary

Add importlib_metadata package implementation

Overview

This PR adds a complete implementation of the importlib_metadata package, which provides APIs for exposing metadata from third-party Python packages. The implementation includes core functionality, compatibility layers, utility modules, and comprehensive test coverage.

Change Types

Type Description
Feature New importlib_metadata package implementation with Distribution, EntryPoint, and metadata handling
Enhancement Added Sphinx documentation configuration and extensions
Test Added comprehensive test suite for the new package

Affected Modules

Module / File Change Description
importlib_metadata/__init__.py Core implementation with Distribution, EntryPoint, PackagePath classes
_adapters.py Message class extending email.message.Message with PEP 566 support
_compat.py Compatibility module for distribution finders
_functools.py Utility functions from jaraco.functools
_itertools.py Collection iteration utilities
_meta.py Protocol classes for package metadata and path handling
_context.py Exception suppression context decorator
_path.py File system structure utilities
compat/py39.py Python 3.9 compatibility layer
compat/py312.py Python 3.12 compatibility with isolated_modules()
conf.py Sphinx documentation configuration updates
conftest.py Pytest configuration for coverage reporting
test_main.py Test suite for package functionality
test_zip.py Tests for zip and egg file handling

Notes for Reviewers

  • The implementation includes several utility modules adapted from jaraco libraries
  • Coverage reporting issues are addressed in the pytest configuration
  • The package includes compatibility layers for different Python versions (3.8, 3.9, 3.12)

jherland and others added 30 commits March 19, 2023 03:44
…URCES.txt

Rename starved_egg to sources_fallback.
The import names that were created by these tests were not valid Python
identifiers.

Fix that, and furthermore: add another check to verify that _all_ import
names returned from packages_distributions() are always valid Python
identifiers. Ideally we should check that all keys returned from
packages_distributions() are valid import names (i.e. can be imported),
but this is at least a step in the right direction.
pep8 states that comments should be limited to 72 characters.
add .get() to the PackageMetadata protocol

Fixes python#384.
…ers.

The main contstraint here for an importable module is that it must not contain a module separator ('.'). Other names that contain dashes or spaces cannot be imported with the 'import' statement, but can be imported with 'importlib.import_module' or invoked with 'runpy'.
Captures new failed expectation and verifies fix.
…port names"

This reverts commit 70ff991.

This behavior was adopted in 5e8260c and subsequently adapted as part of python#443.
…isting files for all the entries in RECORD"

This reverts commit fa9cca4.
…to build the metadata RECORD from the files definition.
Use jaraco.path for generating the record and files
finder, '__module__', None
) == '_frozen_importlib_external' and hasattr(finder, 'find_distributions')

for finder in filter(matches, sys.meta_path): # pragma: nocover
Copy link

Choose a reason for hiding this comment

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

🐛 Correctness Issue

Critical: Monkey-patching stdlib finder breaks compatibility.

Deleting find_distributions from stdlib finders can break compatibility with other packages that rely on standard library behavior.

Current Code (Diff):

-     for finder in filter(matches, sys.meta_path):  # pragma: nocover
-         del finder.find_distributions
+     for finder in filter(matches, sys.meta_path):  # pragma: nocover
+         # Consider safer alternatives like prepending to sys.meta_path instead of modifying stdlib
+         finder.find_distributions = lambda *args, **kwargs: []
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
for finder in filter(matches, sys.meta_path): # pragma: nocover
for finder in filter(matches, sys.meta_path): # pragma: nocover
# Consider safer alternatives like prepending to sys.meta_path instead of modifying stdlib
finder.find_distributions = lambda *args, **kwargs: []

Comment on lines +15 to +17
sys.meta_path.append(cls())
disable_stdlib_finder()
return cls
Copy link

Choose a reason for hiding this comment

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

🐛 Correctness Issue

Critical: Global state modification without cleanup.

The install function modifies global state (sys.meta_path) without providing a way to revert changes, which could cause conflicts with other libraries.

Current Code (Diff):

-     sys.meta_path.append(cls())
-     disable_stdlib_finder()
-     return cls
+     original_meta_path = list(sys.meta_path)
+     sys.meta_path.insert(0, cls())
+     disable_stdlib_finder()
+     return cls
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
sys.meta_path.append(cls())
disable_stdlib_finder()
return cls
original_meta_path = list(sys.meta_path)
sys.meta_path.insert(0, cls())
disable_stdlib_finder()
return cls

@functools.wraps(func)
def wrapper(param, *args, **kwargs):
if param is not None:
return func(param, *args, **kwargs)
Copy link

Choose a reason for hiding this comment

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

🐛 Correctness Issue

Missing return value when param is None.

The pass_none decorator doesn't return a value when param is None, which could cause unexpected behavior for callers expecting a return value.

Current Code (Diff):

        if param is not None:
            return func(param, *args, **kwargs)
+        return None
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
return func(param, *args, **kwargs)
return func(param, *args, **kwargs)
return None

Comment on lines +43 to +46
def get_all(self, name: str, failobj: _T) -> Union[List[Any], _T]:
"""
Return all values associated with a possibly multi-valued key.
"""
Copy link

Choose a reason for hiding this comment

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

🐛 Correctness Issue

Missing ellipsis in @overload method.

The @overload method 'get_all' is missing the required ellipsis (...) after the docstring, which will cause a syntax error.

Current Code (Diff):

-     @overload
-     def get_all(self, name: str, failobj: _T) -> Union[List[Any], _T]:
-         """
-         Return all values associated with a possibly multi-valued key.
-         """
+     @overload
+     def get_all(self, name: str, failobj: _T) -> Union[List[Any], _T]:
+         """
+         Return all values associated with a possibly multi-valued key.
+         """
+         ...  # pragma: no cover
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
def get_all(self, name: str, failobj: _T) -> Union[List[Any], _T]:
"""
Return all values associated with a possibly multi-valued key.
"""
@overload
def get_all(self, name: str, failobj: _T) -> Union[List[Any], _T]:
"""
Return all values associated with a possibly multi-valued key.
"""
... # pragma: no cover

except AttributeError:
from .. import Prepared # -> delay to prevent circular imports.

return Prepared.normalize(getattr(dist, "name", None) or dist.metadata['Name'])
Copy link

Choose a reason for hiding this comment

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

🐛 Correctness Issue

Potential KeyError in metadata access.

Direct access to dist.metadata['Name'] will cause a KeyError exception if the 'Name' key doesn't exist in the metadata dictionary.

Current Code (Diff):

-         return Prepared.normalize(getattr(dist, "name", None) or dist.metadata['Name'])
+         return Prepared.normalize(getattr(dist, "name", None) or dist.metadata.get('Name', ''))
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
return Prepared.normalize(getattr(dist, "name", None) or dist.metadata['Name'])
return Prepared.normalize(getattr(dist, "name", None) or dist.metadata.get('Name', ''))

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.