distutils & setuptools: Relax path related params
#11948
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This has conflicts now. |
This comment has been minimized.
This comment has been minimized.
|
|
||
| @overload |
There was a problem hiding this comment.
Why do we need these overloads? Looks like only base_name and root_dir are different, and when I look at the implementation in 3.11, I don't see any interdependency between those.
There was a problem hiding this comment.
If root_dir is not None, base_name can be PathLike[str]
https://github.com/python/cpython/blob/v3.11.9/Lib/distutils/archive_util.py#L225-L227
There was a problem hiding this comment.
I see. I'm not enthusiastic about adding overloads for cases like this where some types work by accident depending on the path taken through the body of the function.
stdlib/distutils/command/config.pyi
Outdated
| @@ -80,4 +81,4 @@ class config(Command): | |||
| self, header: str, include_dirs: Sequence[str] | None = None, library_dirs: Sequence[str] | None = None, lang: str = "c" | |||
| ) -> bool: ... | |||
|
|
|||
| def dump_file(filename: str, head: Any | None = None) -> None: ... | |||
| def dump_file(filename: FileDescriptorOrPath, head: Any | None = None) -> None: ... | |||
There was a problem hiding this comment.
File descriptors don't work with the log.info('%s', filename) line in the implementation. I suppose we could overload on the value of head but that seems like overkill.
There was a problem hiding this comment.
Doesn't printf-style '%s' % value just coerce to string? https://github.com/python/cpython/blob/v3.11.9/Lib/distutils/log.py#L25
And yeah if FileDescriptorOrPath doesn't fit all code paths for a method, it's overkill to overload it.
Similarly if there's a FileDescriptorOrPath that is technically type-safe but you believe doesn't make sense in context, I don't mind changing it to StrOrBytesPath.
There was a problem hiding this comment.
Oh right, I've been writing too much C where this would segfault. I guess we can allow file descriptors here if they would work at runtime, but I'm sort of dubious on how helpful it is. There's a benefit to adding fds to the allowed types in our stubs (someone could be intentionally passing in an fd) but also a cost (someone could accidentally pass in some random int and now they won't see a type error), and I'm not convinced the benefits outweigh the costs.
Then again, I don't really use distutils, so I'm happy to defer to your judgment.
There was a problem hiding this comment.
I don't really use distutils, so I'm happy to defer to your judgment.
I care most about allowing PathLike wherever possible, even if it's an implementation detail. especially for methods used by, or overridden by, setuptools.
FileDescriptorOrPath is candy on top.
but also a cost (someone could accidentally pass in some random int and now they won't see a type error), and I'm not convinced the benefits outweigh the costs.
but I would personally skip some of the overly precise types that seem to be encoding implementation details.
You make a good point. Eh, I'm not attached to it. I won't keep the filedescriptors params
…ils-relax-path-params
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
This started as wanting to resolve this setuptools TODO: https://github.com/pypa/setuptools/blob/main/setuptools/command/editable_wheel.py#L469-L471
so it can start running mypy on Python 3.12: pypa/setuptools#4352 (comment)
But I ended up going all-out. Doing more than just
copy_fileand its used methods. Especially since I'm not concerned about the implementation changing given thatdistutilsis retired and I'm adding first-party annotations tosetuptools, keeping stubs and library in sync.