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 support for a directory of resources #228

Closed
jaraco opened this issue May 24, 2021 · 6 comments · Fixed by #255
Closed

Add support for a directory of resources #228

jaraco opened this issue May 24, 2021 · 6 comments · Fixed by #255
Assignees

Comments

@jaraco
Copy link
Member

jaraco commented May 24, 2021

Today I was looking at jaraco/cssutils#9 and discovered that cssutils takes advantage of directory tree support in pkg_resources. That is, pkg_resources.resource_filename supports specifying a directory as a resource and making those contents available on the file system.

Currently, the as_file implementation only supports a single file and not a tree of files. To support use-cases like those of cssutils, as_file should support extracting a tree of traversables.

@FFY00
Copy link
Member

FFY00 commented May 24, 2021

I think you meant to open this in importlib_resources, you should be able to transfer the issue there.


Would expanding as_file work in the case you provided? as_file is a context manager and the file is only guaranteed to be on the file system for the duration of it, so you can't return the path as you are doing in get_sheet_filename. I think you need to either change the function to return a traversable, or if you really want a file system path, the __fspath__ proposal would work perfectly here 😁.

@jaraco jaraco transferred this issue from python/importlib_metadata May 24, 2021
@jaraco
Copy link
Member Author

jaraco commented May 24, 2021

I think you meant to open this in importlib_resources, you should be able to transfer the issue there.

Yep. I'm often getting these mixed up.

@jaraco
Copy link
Member Author

jaraco commented May 24, 2021

as_file is a context manager and the file is only guaranteed to be on the file system for the duration of it, so you can't return the path as you are doing in get_sheet_filename.

Right. And that's the core challenge with these file-based providers. If the file (or directory of files) doesn't already exist on the disk, something needs to create them on disk and something needs to later clean up that creation. Hence, the as_file presents a context manager, forcing the consumer to define when and how the context is cleaned up.

That doesn't mean the files can't persist for longer. For example, see how certifi/python-certifi#147 proposes to use as_file to create a context manager and have that context manager cleaned up "at exit".

I fail to see how __fspath__ or os.PathLike helps here. PathLike is simply a protocol that says the object can return a string-representation of the object on the file system. If the object doesn't already exist on the file system, a call to __fspath__ would have to create one and would have no opportunity to clean it up.

Maybe it would help if you could describe how a ZipReader could present something with __fspath__ to address this issue (and maybe others)?

@FFY00
Copy link
Member

FFY00 commented May 24, 2021

You know that test/sheets is not a zip module, you know it is on the file-system, unless you are considering the case of user running the tests. os.fspath works when you know this.

Not every use-case guarantees this, for eg. the certifi one doesn't. __fspath__ would simply not work in those cases, but that does not negate its usefulness in the cases where is does work.

I've had use for it a few times.

Maybe it would help if you could describe how a ZipReader could present something with __fspath__ to address this issue (and maybe others)?

It can't, you'd have to resort as_file or something similar, though I would guess in most cases you should just be able to use a traversable to do what you want instead of having to deal with a fs path.


Just an idea, instead of having as_file support a tree of traversables, what about adding a as_file helper directly to Traversable?

(files(something) / 'some-dir' / 'some-file.txt').as_file()

But that puts some extra burden on implementations using traversables that do not inherit from importlib.abc.Traversable, so it's probably not worth it.

@jaraco
Copy link
Member Author

jaraco commented Jul 3, 2022

Not every use-case guarantees [that a resource is on the file system]

Right. And what I want is a solution that works for resources on the file system and resources by other providers, where if a directory of files is needed, it can be extracted to the file system for the duration of a context.

I have an implementation drafted.

@jaraco jaraco self-assigned this Jul 3, 2022
@jaraco
Copy link
Member Author

jaraco commented Jul 3, 2022

Here's a demo of what I have in mind:

 importlib_resources feature/228-directory-of-resources $ pip-run -q cssutils directory-tree -- -i -c "import importlib_resources as ir, directory_tree; sheets = ir.files('cssutils').joinpath('tests/sheets'); sheets_dir = ir._common._as_tree(sheets);"
>>> with sheets_dir as temp_tree:
...     directory_tree.display_tree(temp_tree)
... 

$ Operating System : Darwin
$ Path : /var/folders/sx/n5gkrgfx6zd91ymxr2sr9wvw00n8zm/T/tmpeemd8e06sheets

*************** Directory Tree ***************

tmpeemd8e06sheets/
├── 096.css
├── 097.css
├── 1.css
├── 1ascii.css
├── 1import.css
├── 1inherit-ascii.css
├── 1inherit-iso.css
├── 1inherit-utf8.css
├── 1utf.css
├── 2inherit-iso.css
├── 2resolve.css
├── acid2.css
├── all.css
├── atrule.css
├── bad.css
├── basic.css
├── bundle.css
├── cases.css
├── csscombine-1.css
├── csscombine-2.css
├── csscombine-proxy.css
├── cthedot_default.css
├── default_html4.css
├── hacks.css
├── html.css
├── html20.css
├── html40.css
├── images/
│   └── example.gif
├── import/
│   ├── images2/
│   │   └── example2.gif
│   ├── import-impossible.css
│   └── import2.css
├── import.css
├── import3.css
├── ll.css
├── ll2.css
├── multiple-values.css
├── page_test.css
├── sample_5.css
├── sample_7.css
├── simple.css
├── single-color.css
├── slashcode.css
├── t-HACKS.css
├── test-unicode.css
├── test.css
├── tigris.css
├── tigris2.css
├── u_simple.css
├── v_simple.css
├── var/
│   ├── start.css
│   ├── use.css
│   ├── vars.css
│   └── vars2.css
├── vars.css
├── varsimport.css
├── xhtml2.css
├── xhtml22.css
└── yuck.css

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 a pull request may close this issue.

2 participants