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

Add a Traversable.read_text() errors parameter #321

Conversation

kurtmckee
Copy link
Contributor

@kurtmckee kurtmckee commented Nov 25, 2024

This PR introduces the following changes, in three commits:

  1. Demonstrate importlib.abc.Traversable.read_text() incompatible with importlib.resources._functional.read_text() usage (Python 3.13) cpython#127012.

    This adds an in-memory finder, loader, and traversable implementation,
    which allows the Traversable protocol and concrete methods to be tested.

    This additional infrastructure demonstrates the original issue,
    but also highlights that the Traversable.joinpath() concrete method
    raises TraversalError which is not getting caught in several places.

  2. Catch TraversalError, raised by Traversable.joinpath().

    Exercising the Traversable protocol's concrete methods
    has highlighted that .joinpath() raises TraversalError,
    which needs to be caught in several places.

    This is primarily resolved within the test suite,
    but implicates the is_resource() function as well.

  3. Resolve a TypeError lurking in the read_text() functional API.

    importlib_resources.read_text() calls the Traversable.read_text()
    concrete method with an errors argument that doesn't exist in the
    method signature, resulting in an TypeError.

    This is resolved by adding an errors parameter to
    Traversable.read_text().

Fixes python/cpython#127012

@kurtmckee kurtmckee changed the title Add a Traversable.readtext() errors parameter Add a Traversable.read_text() errors parameter Nov 25, 2024
@jaraco jaraco self-assigned this Jan 3, 2025
This adds an in-memory finder, loader, and traversable implementation,
which allows the `Traversable` protocol and concrete methods to be tested.

This additional infrastructure demonstrates python/cpython#127012,
but also highlights that the `Traversable.joinpath()` concrete method
raises `TraversalError` which is not getting caught in several places.
Exercising the `Traversable` protocol's concrete methods
has highlighted that `.joinpath()` raises `TraversalError`,
which needs to be caught in several places.

This is primarily resolved within the test suite,
but implicates the `is_resource()` function as well.
`importlib_resources.read_text()` calls the `Traversable.read_text()`
concrete method with an `errors` argument that doesn't exist in the
method signature, resulting in an `TypeError`.

This is resolved by adding an `errors` parameter to
`Traversable.read_text()`.

Fixes python/cpython#127012
@jaraco jaraco force-pushed the add-readtext-errors-parameter-cpython-issue-127012 branch from 509ac17 to d001110 Compare January 3, 2025 16:14
@jaraco
Copy link
Member

jaraco commented Jan 3, 2025

I've force-pushed the three commits after rebasing on main. Now tests are failing because I've re-enabled mypy checks (that were previously disabled).

________________________________________________________ importlib_resources/tests/util.py ________________________________________________________
279: error: Argument 1 to "reduce" has incompatible type "Callable[[dict[str, object], str], object]"; expected "Callable[[dict[str, object], str], dict[str, object]]"  [arg-type]
279: error: Incompatible return value type (got "object", expected "dict[str, object]")  [return-value]
284: error: Argument 1 to "reduce" has incompatible type "Callable[[dict[str, object], str], object]"; expected "Callable[[dict[str, object], str], dict[str, object]]"  [arg-type]
284: error: Incompatible return value type (got "object", expected "dict[str, object]")  [return-value]

@jaraco
Copy link
Member

jaraco commented Jan 3, 2025

In the latest commit, I consolidated the repeated pattern of resolving a fullname from the fixtures, and this incidentally masks the type ambiguity.

@jaraco jaraco force-pushed the add-readtext-errors-parameter-cpython-issue-127012 branch from 30bc726 to 72d550d Compare January 3, 2025 16:38
@jaraco jaraco merged commit 10d87bf into python:main Jan 3, 2025
15 checks passed
@jaraco
Copy link
Member

jaraco commented Jan 3, 2025

Thanks again for this PR. The work you did here was exemplary - it was clear, understandable, complete, and directly addressed the issue. Bravo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants