bpo-30247: Add os.PathLike handle for finder and loader #1422
Conversation
|
@lulouie, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brettcannon, @serhiy-storchaka and @ericsnowcurrently to be potential reviewers. |
Lib/importlib/_bootstrap_external.py
Outdated
| @@ -802,7 +802,7 @@ def __init__(self, fullname, path): | |||
| """Cache the module name and the path to the file found by the | |||
| finder.""" | |||
| self.name = fullname | |||
| self.path = path | |||
| self.path = path.__fspath__() if hasattr(path, '__fspath__') else path | |||
There was a problem hiding this comment.
You can actually use _os.fspath() instead of this little trick.
There was a problem hiding this comment.
ah, is _os, not os, ok.
There was a problem hiding this comment.
@brettcannon why is some class meta using os at the top, but here we need to use _os?
There was a problem hiding this comment.
@brettcannon I think we will still need to use this trick, the test cases including None, list and others , if we pass it to _os.fspath, it will raise the TypeError.
There was a problem hiding this comment.
@lulouie can you point me at the tests that test for None and lists? And if accepting None is acceptable then you simply test for that case, e.g. os.fspath(path) if path is not None else None or something.
There was a problem hiding this comment.
@brettcannon There are some test about None, {} in unittest.
None: test_finder.py
{}: test_file_loader.py
There was a problem hiding this comment.
But they are using empty dict, do they accept something like {'a': path_a, 'b': path_b} ?
Lib/test/test_importlib/util.py
Outdated
| @@ -23,9 +24,11 @@ | |||
|
|
|||
| EXTENSIONS = types.SimpleNamespace() | |||
| EXTENSIONS.path = None | |||
| EXTENSIONS.pathlike = None | |||
There was a problem hiding this comment.
These add attributes aren't really necessary as you can construct the pathlib instances on-demand.
Lib/importlib/_bootstrap_external.py
Outdated
| @@ -802,7 +802,7 @@ def __init__(self, fullname, path): | |||
| """Cache the module name and the path to the file found by the | |||
| finder.""" | |||
| self.name = fullname | |||
| self.path = path | |||
| self.path = path.__fspath__() if hasattr(path, '__fspath__') else path | |||
There was a problem hiding this comment.
@lulouie can you point me at the tests that test for None and lists? And if accepting None is acceptable then you simply test for that case, e.g. os.fspath(path) if path is not None else None or something.
Lib/importlib/_bootstrap_external.py
Outdated
| @@ -908,7 +908,7 @@ class ExtensionFileLoader(FileLoader, _LoaderBasics): | |||
|
|
|||
| def __init__(self, name, path): | |||
| self.name = name | |||
| self.path = path | |||
| self.path = path.__fspath__() if hasattr(path, '__fspath__') else path | |||
There was a problem hiding this comment.
It's probably better to call super().__init__(name, path) than to duplicate this just for extension modules.
Lib/importlib/_bootstrap_external.py
Outdated
| @@ -960,7 +960,7 @@ class _NamespacePath: | |||
|
|
|||
| def __init__(self, name, path, path_finder): | |||
| self._name = name | |||
| self._path = path | |||
| self._path = path.__fspath__() if hasattr(path, '__fspath__') else path | |||
There was a problem hiding this comment.
This should be reverted as _NamespacePath is private and this affects what is valid for __path__ which I'm not prepared to change.
Lib/importlib/_bootstrap_external.py
Outdated
| @@ -1203,7 +1203,7 @@ def __init__(self, path, *loader_details): | |||
| loaders.extend((suffix, loader) for suffix in suffixes) | |||
| self._loaders = loaders | |||
| # Base (directory) path | |||
| self.path = path or '.' | |||
| self.path = path.__fspath__() if hasattr(path, '__fspath__') else path or '.' | |||
There was a problem hiding this comment.
This should become a multi-line statement as there's too much going on to keep it a single line (even if you do switch to _os.fspath() as that or is not obvious anymore).
There was a problem hiding this comment.
what about _os.fspath(path) if path else '.'?
|
@brettcannon so the PathLike test only needs to deal with |
|
Yep, testing FileFinder and FileLoader directly is enough (if you really want to test all the subclasses quickly and simply that's fine too, but don't complicate any of the testing infrastructure to do it). And this will require a doc update and a mention in Misc/NEWS, but as can worry about that once we're happy with the code. |
|
To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request. If/when the requested changes have been made, please leave a comment that says, |
|
The branch for this PR has been deleted. This PR can be closed. @brettcannon |
No description provided.