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

Bypass native readers with files #268

Merged
merged 3 commits into from
Feb 17, 2023
Merged

Bypass native readers with files #268

merged 3 commits into from
Feb 17, 2023

Conversation

jaraco
Copy link
Member

@jaraco jaraco commented Oct 5, 2022

@jaraco
Copy link
Member Author

jaraco commented Oct 5, 2022

Verified the fix against the repro:

$ pip-run sphinxcontrib-htmlhelp git+https://github.com/python/importlib_resources@feature/future-is-here -- -c "import importlib_resources as resources; resources.files('sphinxcontrib').joinpath('htmlhelp', 'templates')" && echo Worked!
Worked 

@jaraco jaraco requested a review from FFY00 October 5, 2022 20:48
@jaraco jaraco closed this Oct 5, 2022
@jaraco jaraco reopened this Oct 5, 2022
@jaraco
Copy link
Member Author

jaraco commented Oct 5, 2022

Ignore the first commit. That's on main now and unrelated.

Copy link
Member

@FFY00 FFY00 left a comment

Choose a reason for hiding this comment

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

Hum, this disregards custom loaders, no? What about simply giving our readers precedence?

@jaraco
Copy link
Member Author

jaraco commented Dec 24, 2022

Hum, this disregards custom loaders, no? What about simply giving our readers precedence?

Excellent point. I think I'd missed that consideration, which probably also means that the test suite could use better coverage to capture that use-case.

Also, your suggestion to simply give precedence to this module's readers might be an excellent approach.

@jaraco
Copy link
Member Author

jaraco commented Feb 17, 2023

In this latest chain, I've pushed a test that passes against main but fails with the patch, capturing the concern FFY00 raised.

@jaraco
Copy link
Member Author

jaraco commented Feb 17, 2023

I've confirmed that the patch still works for the reported failure:

 draft $ pip-run sphinxcontrib-htmlhelp git+https://github.com/python/importlib_resources@main -- -c "import importlib_resources as resources; resources.files('sphinxcontr
ib').joinpath('htmlhelp', 'templates')" && echo Worked!
Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: MultiplexedPath.joinpath() takes 2 positional arguments but 3 were given
 draft $ pip-run sphinxcontrib-htmlhelp git+https://github.com/python/importlib_resources@feature/future-is-here -- -c "import importlib_resources as resources; resources.
files('sphinxcontrib').joinpath('htmlhelp', 'templates')" && echo Worked!
Worked 

@jaraco jaraco merged commit 8da2027 into main Feb 17, 2023
@jaraco jaraco deleted the feature/future-is-here branch February 17, 2023 22:27
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.

files(...).joinpath doesn't accept variable number of arguments in Python 3.10+
2 participants