Replace legacy API implementation with files()

[FFY00]

No description provided.

[FFY00]

@jaraco the first commit is bad, but I wonder if is something that we actually want to do? The Path class is clearly designed to not be used, but it needs to be working because the files() will use it. I think we have two options here.

1. Replace DegenerateFiles.Path with pathlib.Path and force ResourceReader.contents() to iterate valid paths.

2. Make DegenerateFiles.Path try to emulate the normal behavior with non-existent ResourceReader.contents() files.

I would greatly prefeer 1), which actually seems like the correct behavior, if I understand wrap_spec correctly. 2) is a bit tricky to implement, and I am having some issues.

[jaraco]

@FFY00: Can we back up a bit? What expectation is missed that led you to draft the first commit (193ede9)? I see the commit message says "fix DegenerateFiles not exposing loader contents", but I'm unsure why that's needed. Would it be possible to simply deprecate contents altogether?

[FFY00]

Currently, tests are failing if I switch the contents implementation to the files API.

importlib_resources/importlib_resources/tests/test_resource.py

Line 56 in 9822338

classResourceLoaderTests(unittest.TestCase):

The current contents implementation will work fine if we create a module manually as done in tests.util.create_package, the files API will not. It will return an empty iterable because that is what the DegenerateFiles.Path implementation does.

importlib_resources/importlib_resources/tests/util.py

Line 54 in e3b5f8d

defcreate_package(file, path, is_package=True, contents=()):

We could simply deprecate contents, but as discussed in #80, I am reimplementing the legacy API with files to make sure there is no edge case where it doesn't work, and as discovered here, there is.
I guess the question now is, do we care about this case? If we do, do we handle it in the files API, or do we come up with other solution?
To me, it seems wrong that DegenerateFiles will return a traversable that does not correctly represent the spec contents.

[jaraco]

If DegenerateFiles.Path implements this behavior, then it betrays its namesake. That is, it's no longer degenerate if it's passing through to some underlying implementation (i.e. ResourceReader.contents).

Maybe the right thing to do here is to rename DegenerateFiles to CompatibilityFiles or FallbackFiles and then allow it to grow this functionality.

An important thing to remember regarding the ResourceReader is that it's an abstract concept. The tests here are capturing an abstract concept that other loaders may be implementing. So option (1) isn't viable (you can't assume the contents of a module could be represented by a pathlib.Path object).

As I see it, the question is - what should happen for a loader that implements 'contents' but not 'files' - should we wrap that implementation and create a files-like interface? My instinct is no... but that will break compatibility, and it's not even be possible in the general case (files implements features that the other APIs were incapable of supporting).

I think you're struggling with a lot of the challenges I've struggled with while trying to adapt everything to run on files().

I don't have a good recommendation at this time, but I'll try to loop back on it again later... and I welcome you to continue to explore it.

[FFY00]

Maybe the right thing to do here is to rename DegenerateFiles to CompatibilityFiles or FallbackFiles and then allow it to grow this functionality.

I agree.

As I see it, the question is - what should happen for a loader that implements 'contents' but not 'files' - should we wrap that implementation and create a files-like interface? My instinct is no... but that will break compatibility, and it's not even be possible in the general case (files implements features that the other APIs were incapable of supporting).

The thing is, this is already supported by the legacy API. I gave it another try and the issue I am running into is a lack of means to implement is_file, exists, is_dir -- they would need to be supported by the resource reader. Which is essentially the issue you described.

We could support these cases in the files API by making the returned traversable raise an unsupported operation error on such scenarios, which I feel is not a particularly bad idea.
The legacy contents API returns strings, so in files we could return traversables that would still hold that string value but have limited value besides that, we could still support open where ResourceReader can do it, but not is_file and friends.

This proposal makes it possible to easily replace contents usages with files, the possible issue here would be this somehow damaging expectations of the files API. But IMO this should not really be a big issue, as it is an edge case, and we could possibly add support to ResourceReader to optionally implement the missing functionality.
What do you think?

[FFY00]

I have a successful implementation that uses the approach I described.

[jaraco]

we could possibly add support to ResourceReader to optionally implement the missing functionality

Since other resource providers implement the the concrete ResourceReader subclasses, any change would need to be compatible with the existing interface, so I suspect this approach is not worthwhile.

I notice the diffcov tests are failing with:

importlib_resources/_adapters.py (52.2%): Missing lines 28,40,56,61-62,65-66,69,76-77,80

Not a hard requirement, but I'd prefer if we had tests covering those code paths.

[FFY00]

Since other resource providers implement the the concrete ResourceReader subclasses, any change would need to be compatible with the existing interface, so I suspect this approach is not worthwhile.

Yes, makes sense, we want people moving away from ResourceReader to TraversableReader so extending ResourceReader does not make that much sense.

Discussion moved to #226.

[FFY00]

This is still missing tests for CompatibilityFiles, but is ready for review.

[FFY00]

@jaraco friendly ping. Please have a look when you have time, I know it's a pretty big, and involved, change so don't feel pressured 😅

[FFY00]

@jaraco friendly ping.

[jaraco]

I haven’t forgotten, but I’ve had quite a lot on my plate. Will get to it soon. Thanks for the patience and reminder.

[jaraco]

This work is excellent. It's clean, makes some smart decisions, but most importantly sets us up to be able to deprecate the legacy API. I have only the one serious reservation, and even that isn't a blocker to move forward, so I'll defer to your preference on whether to pursue those changes in this PR or a subsequent one.

[jaraco]

Big thanks Filipe. Releasing as v5.2.0.