Skip to content

Bypass native readers with files#268

Merged
jaraco merged 3 commits intomainfrom
feature/future-is-here
Feb 17, 2023
Merged

Bypass native readers with files#268
jaraco merged 3 commits intomainfrom
feature/future-is-here

Conversation

@jaraco
Copy link
Copy Markdown
Member

@jaraco jaraco commented Oct 5, 2022 •
edited
Loading

@jaraco
Copy link
Copy Markdown
Member Author

jaraco commented Oct 5, 2022 •
edited
Loading

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
Copy Markdown
Member Author

jaraco commented Oct 5, 2022

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

FFY00
FFY00 reviewed Oct 5, 2022
View reviewed changes
Copy link
Copy Markdown
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 jaraco force-pushed the feature/future-is-here branch from ed8fa45 to 9396ffc Compare December 24, 2022 18:09
@jaraco
Copy link
Copy Markdown
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 jaraco force-pushed the feature/future-is-here branch from 9396ffc to 68be8b4 Compare February 17, 2023 21:40
@jaraco
Copy link
Copy Markdown
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
Copy Markdown
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 added a commit that referenced this pull request Feb 17, 2023
@jaraco
Merge pull request #268 from python/feature/future-is-here
7f4fbb5
@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

Reviewers

@FFY00 FFY00 FFY00 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

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

@jaraco @FFY00