python / cpython

The Python programming language
https://www.python.org
Other
62.45k stars 29.97k forks source link

Recommend importlib.abc.Traversable users to implement __fspath__ #88366

Open FFY00 opened 3 years ago

FFY00 commented 3 years ago
BPO 44200
Nosy @brettcannon, @jaraco, @FFY00

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['3.10', 'docs'] title = 'Recommend importlib.abc.Traversable users to implement __fspath__' updated_at = user = 'https://github.com/FFY00' ``` bugs.python.org fields: ```python activity = actor = 'FFY00' assignee = 'docs@python' closed = False closed_date = None closer = None components = ['Documentation'] creation = creator = 'FFY00' dependencies = [] files = [] hgrepos = [] issue_num = 44200 keywords = [] message_count = 6.0 messages = ['394083', '394084', '394211', '394213', '394219', '394223'] nosy_count = 4.0 nosy_names = ['brett.cannon', 'jaraco', 'docs@python', 'FFY00'] pr_nums = [] priority = 'normal' resolution = None stage = None status = 'open' superseder = None type = None url = 'https://bugs.python.org/issue44200' versions = ['Python 3.10'] ```

FFY00 commented 3 years ago

The files()/Traversable api is meant to replace ResourceReader api, but it falls short in one dimension, tying the abstraction to a file system path.

I propose to document that Traversable users *should* implement __fspath__ if the Traversable represents a file-system path.

This will essentially make os.fspath(importlib.resources.files(module) / resource) the equivalent of importlib.resources.path(module, resource), for which currently there is no replacement using files().

FFY00 commented 3 years ago

I am gonna wait until/if python/cpython#70460 (bpo-44196) gets merged because this would conflict with that.

jaraco commented 3 years ago

The problem with the __fspath__ protocol is that it assumes a file system. The importlib.resources and Traversable protocols are trying to provide a lower-level interface that doesn't assume a file system. Fortunately, there already exists a helper function as_file (https://docs.python.org/3/library/importlib.html#importlib.resources.as_file) that's meant to provide the user experience that path once provided, but for Traversable files.

Does that satisfy the need you've identified?

FFY00 commented 3 years ago

That's why I said it should be optional. If the object can implement it, it should. pathlib.Path certainly can, zipfile.Path can't, and that's alright. If the Traversable does not represent a path on the file-system, it does not make sense to implement __fspath__, so it shouldn't.

I think as_file should learn to behave like path, if __fspath__ is available, it will use that path, otherwise it will do what it does currently, which is creating a temporary path and writing the resource contents there.

jaraco commented 3 years ago

I think as_file should learn to behave like path, if __fspath__ is available, it will use that path.

Oh, that's an interesting idea. And that's effectively what happens here, where as_file returns a native Path object if that's what it encounters. Maybe another dispatcher could accept os.PathLike and in that case, return pathlib.Path(os.fspath(path)).

Only thing is, that code would serve no purpose until there existed at least one other resource provider besides FileReader that has file-system-local resources. I'm not aware of any such provider.

FFY00 commented 3 years ago

CompatibilityFiles would use this. We could add a dispatch for that too, but that's 2 different cases now and os.PathLike would be able to handle both.

moomoohk commented 2 years ago

Hey, I'm a bit confused as to why Traversable was ever implemented in the first place. Seems like Traversable is supposed to represent file system paths, however I was sure that's why we have Paths.

Whatever Traversable's added value is, I still think the class should be coupled to Path type-wise. Maybe by marking Traversable as PathLike.

At the moment, functions like importlib.resources.files specify a return type of Iterator[Traversable] but in reality return Iterator[Path]. This results in confusing type hint warnings since IDEs have no way to know that Traversable and Path are related.

I'm not very experienced with the newer versions of importlib so my assumptions might be a bit off, hoping one of could shed some light.

Thanks

jaraco commented 6 months ago

CompatibilityFiles would use this. We could add a dispatch for that too, but that's 2 different cases now and os.PathLike would be able to handle both.

I'm not sure I understand the two cases. Perhaps you could illustrate how this would work within CompatibilityFiles?

FFY00 commented 5 months ago

CompatibilityFiles is an abstraction over ResourceReader, it implements a Traversable interface using ResourceReader.contents, ResourceReader.resource_path, etc. In this case, the resource file can exist on the filesystem, but we cannot simply return a pathlib.Path of the root of the package in files(), as the reader might customize the available resources.

Currently, if I need to access a resource as a file on the filesystem, I need to use as_file or something similar, which will write the resource contents to a temporary file. This isn't optimal, so to avoid it, we could register another dispatch method in as_file to special-case CompatibilityFiles, like we do for pathlib.Path:

https://github.com/python/importlib_resources/blob/6fa0a7aef25178430816c4459524a4945bfb7ee1/importlib_resources/_common.py#L173-L179

But I feel like a better solution would be to have the Traversable in question, which in this example is CompatibilityFiles, implement __fspath__ if it is representing a path on the filesystem. This has two benefits: