msg413142 - (view) |
Author: Barney Gale (barneygale) * |
Date: 2022-02-12 19:35 |
The docs for NotImplementedError say:
> In user defined base classes, abstract methods should raise this exception when they require derived classes to override the method, or while the class is being developed to indicate that the real implementation still needs to be added.
pathlib's use of NotImplementedError appears to be more broad. It can be raised in the following circumstances:
1. When attempting to construct a WindowsPath from a non-Windows system, and vice-versa. This is the only case where NotImplementedError is mentioned in the pathlib docs (in a repl example)
2. In glob() and rglob() when an absolute path is supplied as a pattern
3. In owner() if the pwd module isn't available
4. In group() if the grp module isn't available
5. In readlink() if os.readlink() isn't available
6. In symlink_to() if os.symlink() isn't available
7. In hardlink_to() if os.hardlink() isn't available
8. In WindowsPath.is_mount(), unconditionally
I suspect there are better choices for exception types in all these cases.
|
msg413143 - (view) |
Author: Alex Waygood (AlexWaygood) *  |
Date: 2022-02-12 20:24 |
Here's my two cents, as a non-expert when it comes to pathlib:
I'm not really sure why `is_mount()` exists on WindowsPath objects, given that it unconditionally raises `NotImplementedError` on WindowsPath objects -- that seems *very* strange to me. It seems to me like it should probably be a method on the `PosixPath` class rather than on the `Path` class.
The other methods that raise NotImplementedError don't seem as egregious, because none of them *unconditionally* raise NotImplementedError. We could debate whether some of them would raise different errors in an ideal world, but changing them now might have implications for backwards compatibility, and it doesn't seem like a *major* issue to me.
|
msg413144 - (view) |
Author: Alex Waygood (AlexWaygood) *  |
Date: 2022-02-12 20:28 |
I suppose it might also be worth considering moving `owner()` and `group()` to PosixPath. In practice, these unconditionally raise NotImplementedError on Windows, since the pwd and grp modules are not available on Windows. So, in an ideal world, they probably wouldn't exist on the common base class.
Again, though, that needs to be weighed against backwards-compatibility considerations.
|
msg413263 - (view) |
Author: Barney Gale (barneygale) * |
Date: 2022-02-14 21:01 |
Thanks very much Alex. I've added some PRs:
PR 31338 addresses owner(), group() and is_mount(). It moves those methods to PosixPath, and adds stubs in WindowsPath that raise deprecation warnings. I agree with your analysis that, for static typing purposes, these methods shouldn't even exist on WindowsPath!
PR 31339 addresses readlink(), symlink_to() and hardlink_to(). In this case I'm working towards making those methods unavailable if os.readlink(), symlink() and link() are unavailable. Not totally sold on this - thoughts?
PR 31340 addresses glob() and rglob(), switching the exception type to ValueError. I think this is a legitimate bugfix with minimal adverse effects.
PR 31341 addresses the Path constructor. This is a backwards incompatible change, and /probably/ not worth doing. I add it for completeness sake, as these four PRs cover all cases where pathlib raises NotImplementedError.
|
msg413276 - (view) |
Author: Eryk Sun (eryksun) *  |
Date: 2022-02-15 04:25 |
WindowsPath.is_mount() should call ntpath.ismount(). This function needs a significant redesign, but it's fine to use it as long as it's documented that is_mount() is equivalent to os.path.ismount().
Since the owner() and group() methods return names instead of numeric IDs, technically we could implement them in WindowsPath. That said, in Windows, st_mode and chmod() are unrelated to a file's owner and group, plus object ownership is fundamentally different from POSIX. Unless chown() is also implemented, I don't think we would gain much from knowing the owner and group, other than making it easier to display them.
|
msg413298 - (view) |
Author: Barney Gale (barneygale) * |
Date: 2022-02-15 17:17 |
I'm planning to learn more heavily on posixpath + ntpath in pathlib once bpo-44136 is done. I think that would be a good time to introduce is_mount() support on Windows.
|
msg413308 - (view) |
Author: Eryk Sun (eryksun) *  |
Date: 2022-02-16 01:49 |
> I'm planning to learn more heavily on posixpath + ntpath in
> pathlib once bpo-44136 is done. I think that would be a good
> time to introduce is_mount() support on Windows.
In the long run, it would be better to migrate the implementations in the other direction. Rewrite genericpath, ntpath, posixpath, and parts of shutil to use PurePath and Path objects. Using path objects instead of generic strings should improve the implementation of os.path and shutil, in addition to ensuring consistency with pathlib. However, that's a long-term goal that doesn't preclude using os.path and shutil as needed now, such as relying on os.path.ismount().
|
msg413506 - (view) |
Author: Terry J. Reedy (terry.reedy) *  |
Date: 2022-02-18 19:48 |
I think pathlib should be raising TypeError, ValueError, or something else as appropriate. Or not raising in situations Eryk indicated. x/0 *could* be +- float('inf'), but we don't raise NotImplementedError for it.
|
msg413627 - (view) |
Author: Barney Gale (barneygale) * |
Date: 2022-02-21 02:08 |
Thanks both. I've adjusted PR 31338 to leave is_mount() unchanged. I've opened a new pull request that implements is_mount() on Windows using ntpath.ismount(): PR 31458
|
msg413644 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2022-02-21 10:33 |
> In the long run, it would be better to migrate the implementations in the other direction. Rewrite genericpath, ntpath, posixpath, and parts of shutil to use PurePath and Path objects.
pathlib does not allow to distinguish "path" from "path/".
|
msg413647 - (view) |
Author: Eryk Sun (eryksun) *  |
Date: 2022-02-21 11:42 |
> pathlib does not allow to distinguish "path" from "path/".
os.path.normpath() and os.path.abspath() don't retain a trailing slash -- or a leading dot component for that matter. Are you referring to os.path.join()? For example:
>>> os.path.join('./spam', 'eggs/')
'./spam/eggs/'
>>> os.path.normpath('./spam/eggs/')
'spam/eggs'
>>> PurePath('./spam') / PurePath('eggs/')
PurePosixPath('spam/eggs')
A leading dot component is significant in a context that searches a set of paths -- usually PATH. A trailing slash is significant in a context that has to distinguish a device or file path from a directory path, of which there are several cases in Windows.
I think it's a deficiency in pathlib that it lacks a way to require conservative normalization in these cases. Path and PurePath objects could gain a keyword-only parameter, and internal attribute if needed, that enables a more conservative normalization.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:56 | admin | set | github: 90889 |
2022-02-21 11:42:45 | eryksun | set | messages:
+ msg413647 |
2022-02-21 10:33:08 | serhiy.storchaka | set | messages:
+ msg413644 |
2022-02-21 02:08:14 | barneygale | set | messages:
+ msg413627 |
2022-02-18 19:48:41 | terry.reedy | set | nosy:
+ terry.reedy messages:
+ msg413506
|
2022-02-16 01:49:24 | eryksun | set | messages:
+ msg413308 |
2022-02-15 17:17:22 | barneygale | set | messages:
+ msg413298 |
2022-02-15 04:25:32 | eryksun | set | nosy:
+ eryksun messages:
+ msg413276
|
2022-02-14 21:01:14 | barneygale | set | messages:
+ msg413263 |
2022-02-14 19:55:37 | barneygale | set | pull_requests:
+ pull_request29492 |
2022-02-14 19:55:33 | barneygale | set | pull_requests:
+ pull_request29491 |
2022-02-14 19:55:29 | barneygale | set | pull_requests:
+ pull_request29490 |
2022-02-14 19:55:14 | barneygale | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request29489 |
2022-02-13 18:12:42 | eric.araujo | set | nosy:
+ pitrou, eric.araujo, serhiy.storchaka
|
2022-02-12 20:28:10 | AlexWaygood | set | messages:
+ msg413144 |
2022-02-12 20:24:53 | AlexWaygood | set | nosy:
+ AlexWaygood messages:
+ msg413143
|
2022-02-12 20:15:13 | AlexWaygood | set | keywords:
- patch stage: patch review -> (no value) |
2022-02-12 20:14:54 | AlexWaygood | set | pull_requests:
- pull_request29461 |
2022-02-12 19:38:00 | barneygale | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request29461 |
2022-02-12 19:35:13 | barneygale | create | |