-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
Un-deprecate functional API for importlib resources & add subdirectory support #116608
Comments
Wouldn't this be a bit late for that? We already went through the deprecation period, and removed the feature in the alpha releases, bringing them back now would be a bit confusing.
Can you actually show a couple examples of this affecting users downstream? I think that's the most viable argument to bring that API back. |
Why not just take multiple components with separators in a single argument? It's easy enough to require forward slash, disallow If they didn't allow subdirectories before (I never noticed, tbh), then presumably using a slash here would have either failed completely or worked. Either way, we can enable them in a new release. (And add me to the anecdotal list of people who missed them. It's easy enough to add a few lines of code to bring them back, which is how I have been handling it so far, but I'd be happier to have those few lines in the stdlib.) |
At least one audience that would like to keep the legacy APIs is in mesonbuild/meson#12401. I admit, I prefer this approach over keeping the legacy APIs with the cruft that it still had lying around. It adds a mostly-compatible layer and restores these wrappers in a supported way. On one hand, this approach violates the "preferably one way" to do things; users will need to decide which way works best for them, creating a variety of supported approaches. On the other hand, I do appreciate that it offers a friendlier interface for certain operations (esp. FFY00 and I put a lot of work into this deprecation process, so it'll be disappointing to now see this reversed at the last minute, but it does feel like the right thing to do, especially since someone else is willing to own the implementation (thanks encukuo!). We will have to backport the change to importlib_resources, but that should be fairly straightforward. Overall, I'm +0 on the change. I'd really like to see more vocal support from other core devs before committing to this approach. |
I've made the encoding argument mandatory for
Yes, sorry. Previously I couldn't commit to supporting this API.
I'd rather not derail discussion on this issue. Support for separators can be added later if necessary. If they will, allowing multiple arguments will still be useful.
Sorry to hear that. Sunk costs suck :( |
I’ll add a “me too” here as well. Being able to do simple things simply is an advantage. |
+1 from me on the (updated) proposed API as well as un-deprecating these -- for reasons that have been discussed on the d.p.o thread as well as mentioned here by others. |
I've always sort of wondered why this is a drawback at all, compared to simply doing this: with importlib.resources.path('modulename.subdirectory.subsubdir', 'resources.txt') as f:
... I'm not objecting to the new API! It's more ergonomic than pretending everything is a namespace module. But for backwards compatibility with python < 3.13 it seems practical to use the two-argument form, and the lack of a new API doesn't seem like it should have been a killer problem before now. |
I don't understand the reason we can't reimplement it as:
Why do we need the module and filename as multiple args instead of just two? |
How is it derailing this issue? You're bringing back an API, which I like, and changing the design in a potentially backwards-incompatible way in the process, which I don't. Why is it derailing to ask why it has to have a different design now? |
I'm catching myself up on this. I think the answer is (somewhere) in this thread: https://gitlab.com/python-devs/importlib_resources/-/issues/58 |
Which was migrated to python/importlib_resources#58. |
IIUC:
So there's perhaps four levels of support we could offer for the functional APIs:
Personally I'd lean towards option 2. If folks need subdirectory support they can use the OOP API - that's it's whole reason to exist! |
Early in the design, I'd deemed it infeasible implement |
I'm going to backtrack slightly on my support for this. I do find the The complexities around "add subdirectory support" are where I'm less certain. I like the idea of supporting subdirectories without needing the full
I would be happy with that. I will note that option 2 was essentially what a lot of people asked for in the Discourse thread at the time of the original deprecation, and it felt very much to me that positions got entrenched because the comments were possibly too heated. Maybe with the passage of time, and experience of the impact, all we're doing here is confirming that it turned out that the wrong choice was made? Luckily, we still have time to reverse that choice before the functional API actually gets removed. |
One issue that was discussed in comments leading up to this comment was that, by adding support for subdirectories, functions like |
I don't see how - you shouldn't be accepting untrusted input to the |
I'm not convinced it's a concern meriting intervention, but it was something that @warsaw raised. |
From the linked comment:
If I'm right in thinking that |
Maybe, but in cases where that would work, there are easier ways for someone to do it. Much easier to say that resources shouldn't be looked up using untrusted input. The vast majority of uses will be using static strings here anyway, because arbitrary files do not exist in your own package. But I believe it uses a different type anyway and only converts to an actual path at the very end, so it may not even be possible to traverse outside of the package. Which makes this entirely a moot point (and would be a great design if so). |
I'm pretty sure it produces a traversable that also inherits from Path and can be used as either one (modulo isinstance checks to see whether it's from pathlib or zipfile for cases where the difference matters). I'd assume that the standard path joining operations use the traversable impl which then checks for attempts to escape the resource root. |
Right, there's no reason someone couldn't be passing the same inputs to os.path.join and then to builtin open() directly, instead of importlib.resources, so it's basically splitting hairs to say that the resources API needs super special protections for security. ... Also a distraction from the main topic I guess. |
@jaraco, do you think allowing path separators is OK? I'm happy to delegate the decision to you. My opinion, for what it's worth: it is OK. I prefer allowing multiple |
In actuality, all
Correct.
Yes, I believe it's okay. I believe there's existing precedence for supporting it, in part because 'pathlib.Path' supports it, but also because it's explicitly documented that Traversables should support paths separated by
Since One more comment - you may consider contributing this change to importlib_resources simultaneous to or prior to submitting it with CPython as importlib_resources provides a fuller test suite, providing coverage on older Pythons and checks for other aspects like code coverage, type checking, style consistency, and more. Totally your call, though. |
Oh, that's great! I'll add it to the docs. The semantics are clear then: I'll delegate to joinpath :)
Roger, will send the patch there too. |
Thanks for the discussion everyone, and sorry I couldn't make everyone happy. I merged the PR. If you disagree with that, please preferably continue the discussion here: https://discuss.python.org/t/11386/47 |
Apparently the tests fail on big-endian machines; will fix |
…GH-117569) gh-116609: Ignore UTF-16 BOM in importlib.resources._functional tests To test the `errors` argument, we read a UTF-16 file as UTF-8 with "backslashreplace" error handling. However, the utf-16 codec adds an endian-specific byte-order mark, so on big-endian machines the expectation doesn't match the test file (which was saved on a little-endian machine). Use endswith to ignore the BOM.
…dd subdirectory support (pythonGH-116609)
… tests (pythonGH-117569) pythongh-116609: Ignore UTF-16 BOM in importlib.resources._functional tests To test the `errors` argument, we read a UTF-16 file as UTF-8 with "backslashreplace" error handling. However, the utf-16 codec adds an endian-specific byte-order mark, so on big-endian machines the expectation doesn't match the test file (which was saved on a little-endian machine). Use endswith to ignore the BOM.
…_metadata. (pythonGH-123028) (cherry picked from commit e913d2c) Co-authored-by: Jason R. Coombs <[email protected]>
… from importlib_resources. (pythonGH-123028) (python#123051)" This reverts commit 5ac14ee. This commit should be re-applied after 3.13.0 final.
…nd compatibility changes from importlib_resources. (pythonGH-123028) (python#123051)" This reverts commit 5ac14ee. This commit should be re-applied after 3.13.0 final.
…pythongh-116608: Apply style and compatibility changes from importlib_resources. (pythonGH-123028) (python#123051)"" This reverts commit 0a058bc.
* gh-116608: Apply style and compatibility changes from importlib_metadata. * gh-121735: Ensure module-adjacent resources are loadable from a zipfile. * gh-121735: Allow all modules to be processed by the ZipReader. * Add blurb * Remove update-zips script, unneeded. * Remove unnecessary references to removed static fixtures. * Remove zipdata fixtures, unused.
…H-123037) * pythongh-116608: Apply style and compatibility changes from importlib_metadata. * pythongh-121735: Ensure module-adjacent resources are loadable from a zipfile. * pythongh-121735: Allow all modules to be processed by the ZipReader. * Add blurb * Remove update-zips script, unneeded. * Remove unnecessary references to removed static fixtures. * Remove zipdata fixtures, unused. (cherry picked from commit ba687d9) Co-authored-by: Jason R. Coombs <[email protected]>
…ythonGH-123037) * pythongh-116608: Apply style and compatibility changes from importlib_metadata. * pythongh-121735: Ensure module-adjacent resources are loadable from a zipfile. * pythongh-121735: Allow all modules to be processed by the ZipReader. * Add blurb * Remove update-zips script, unneeded. * Remove unnecessary references to removed static fixtures. * Remove zipdata fixtures, unused. (cherry picked from commit ba687d9) Co-authored-by: Jason R. Coombs <[email protected]>
…ythonGH-123037) * pythongh-116608: Apply style and compatibility changes from importlib_metadata. * pythongh-121735: Ensure module-adjacent resources are loadable from a zipfile. * pythongh-121735: Allow all modules to be processed by the ZipReader. * Add blurb * Remove update-zips script, unneeded. * Remove unnecessary references to removed static fixtures. * Remove zipdata fixtures, unused. (cherry picked from commit ba687d9) Co-authored-by: Jason R. Coombs <[email protected]>
) (#123986) * gh-121735: Fix module-adjacent references in zip files (GH-123037) * gh-116608: Apply style and compatibility changes from importlib_metadata. * gh-121735: Ensure module-adjacent resources are loadable from a zipfile. * gh-121735: Allow all modules to be processed by the ZipReader. * Add blurb * Remove update-zips script, unneeded. * Remove unnecessary references to removed static fixtures. * Remove zipdata fixtures, unused. (cherry picked from commit ba687d9) * gh-123994: Generate utf-16 file using little endian and BOM. (#123995) Co-authored-by: Jason R. Coombs <[email protected]>
Feature or enhancement
Proposal:
The
importlib.resources
functions{open,read}_{text,binary}
,path
,is_resource
andcontents
, deprecated in 3.11 and removed in 3.13 alphas, are, anecdotally, missed by quite a few users.They provide a simple API for simple tasks, while the full-featured
Traversable
API is better suited for complex ones -- especially for implementing new resources-aware loaders.I'm now in a position where I can add these functions back and support them.
Their main drawback -- not allowing subdirectories -- can be solved by taking multiple path components as positional arguments, for example:
The additional arguments (encoding and errors) would become keyword-only.
There is a wrinkle in this: in Python 3.9-3.11, the above would mean:
I believe that this is acceptable, since:pragmatically: typical file names do not match typical encoding/errorhandler nameslawyerly: the functions have already been deprecated for 2 releases; no one is using them now, right?However, if this is a problem, I can[edit: This is solved by:]
encoding
argument required if a text-reading function more than one path component is given.Has this already been discussed elsewhere?
I have already discussed this feature proposal on Discourse
Links to previous discussion of this feature:
https://discuss.python.org/t/deprecating-importlib-resources-legacy-api/11386/29
Linked PRs
The text was updated successfully, but these errors were encountered: