getnikola / nikola

A static website and blog generator
https://getnikola.com/
MIT License
2.58k stars 443 forks source link

replacing pkg_resources with importlib #3749

Closed fondbcn closed 5 months ago

fondbcn commented 5 months ago

Pull Request Checklist

Description

Sorry for the old error. I updated the file with this PR instead.

Kwpolska commented 5 months ago

There are some more places where pkg_resources is used, could you fix them all?

fondbcn commented 5 months ago

There are some more places where pkg_resources is used, could you fix them all?

not in nikola.py ?

Kwpolska commented 5 months ago

There are some more places where pkg_resources is used, could you fix them all? not in nikola.py ?

Yes, there are 6 other files.

fondbcn commented 5 months ago

There are some more places where pkg_resources is used, could you fix them all? not in nikola.py ?

Yes, there are 6 other files.

how to deal with python 3.8 as felixfontein said ?

felixfontein commented 5 months ago

I would probably create a helper function (in utils.py maybe?) which uses the functions available in Python 3.8. The next step would be to make this function depend on the Python version, since Python 3.11 basically deprecated everything in importlib.resources that was available in Python 3.8...

felixfontein commented 5 months ago

Another alternative might be using https://docs.python.org/3/library/pkgutil.html#pkgutil.get_data instead.

fondbcn commented 5 months ago

Another alternative might be using https://docs.python.org/3/library/pkgutil.html#pkgutil.get_data instead.

will "pkgutil" be widely compatible ?

felixfontein commented 5 months ago

According to the docs it's been available since at least Python 2.6.

fondbcn commented 5 months ago

hoping this works. strangely some files require str() !

Kwpolska commented 5 months ago

Please fix the failing tests and linting errors.

fondbcn commented 5 months ago

Please fix the failing tests and linting errors.

I have no idea why resources.path() is not working!

Kwpolska commented 5 months ago

To understand the 3.8 test failures, you can try running the tests locally, but first changing the code so that the 3.8 version runs on all Python versions (or you could install 3.8, but it’s probably not worth it). The test failures in 3.9+ are unrelated to your code (#3753).

fondbcn commented 5 months ago

To understand the 3.8 test failures, you can try running the tests locally, but first changing the code so that the 3.8 version runs on all Python versions (or you could install 3.8, but it’s probably not worth it). The test failures in 3.9+ are unrelated to your code (#3753).

I tried every method in https://docs.python.org/3.8/library/importlib.html#module-importlib.resources , but nothing is working on 3.8 ! how newer files().joinpath() avoid this issue ???

felixfontein commented 5 months ago

The documentation of importlib.resources.path() says:

Return the path to the resource as an actual file system path. This function returns a context manager for use in a with statement. The context manager provides a pathlib.Path object.

(Emphasis is mine.)

So it returns a context manager, not directly a path. Your code is treating it as a function which returns a path. That doesn't work. Instead of return str(resources.path(package, resource)), you have to do

        with resources.path(package, resource) as path:
            return str(path)

This only works for resources that are not part of an archive. For this to work with resources in an archive, you need to implement a caching solution. Which might be easier if not all uses of resource_filename would be routed through the same utils function call.

I guess one question is how much complexity we want to add for Python 3.8. @Kwpolska what do you think?

Kwpolska commented 5 months ago

This only works for resources that are not part of an archive. For this to work with resources in an archive, you need to implement a caching solution. Which might be easier if not all uses of resource_filename would be routed through the same utils function call.

What’s an archive? We don’t support .egg or zipfile.

If the with statement is enough to fix 3.8, we should do that. If it isn’t, we can use pkg_resources or a backport of importlib_resources on 3.8 (but only 3.8, with a conditional requirement)

felixfontein commented 5 months ago

If we don't support zipfiles or directly running from eggs, then yeah, just doing a return inside the with should be fine.

fondbcn commented 5 months ago

If we don't support zipfiles or directly running from eggs, then yeah, just doing a return inside the with should be fine.

I tried the "with" on path() but the test is still failing. The documentation of importlib.resources (3.9+) says that path() is equivalent to as_file(files().joinpath()), however read() also didn't work.

fondbcn commented 5 months ago

I found a solution that works for all 3.8 and 3.9+ (at least for me), just adding some init.py in data/ and removing os.path.join . However the test time is longer.

Kwpolska commented 5 months ago

Please fix the test failures.

Kwpolska commented 5 months ago

Thanks for your contribution!