Skip to content

Bypass native readers with files - #268

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

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

Conversation

@jaraco

@jaraco jaraco commented Oct 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

@jaraco

jaraco commented Oct 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

jaraco commented Oct 5, 2022

Copy link
Copy Markdown
Member Author

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

FFY00
FFY00 reviewed Oct 5, 2022
View reviewed changes

@FFY00 FFY00 left a comment

Copy link
Copy Markdown
Member

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

jaraco commented Dec 24, 2022

Copy link
Copy Markdown
Member Author

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

jaraco commented Feb 17, 2023

Copy link
Copy Markdown
Member Author

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

@jaraco

jaraco commented Feb 17, 2023

Copy link
Copy Markdown
Member Author

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