Runtime changes for typeshed parameter annotations merge #4505
Conversation
Avasam
force-pushed
the
runtime-changes-for-typeshed-merge
branch
from
dd3af1c to
f7b0e67
Compare
Avasam
changed the title
setuptools/command/easy_install.py
Outdated
| return cls.from_environment() | ||
| # otherwise, assume it's a string. | ||
| return cls.from_string(param) | ||
| return cls.from_environment() |
There was a problem hiding this comment.
If this gracious fallback is undesired, I can explicitly raise instead.
There was a problem hiding this comment.
Let's raise it explicitly because before cls.from_environment() was only used in the case of None. I suspect that the previous fallback to cls.from_string would cause a TypeError if the user passed non-sensical args.
We propbably don't want to change that behaviour
There was a problem hiding this comment.
Raising an error instead of a fallback makes sense since the param unexpectedly goes unused.
It was an AttributeError because non-string first argument to shlex is assumed to be a stream.
File "C:\Program Files\Python39\lib\shlex.py", line 140, in read_token
nextchar = self.instream.read(1)
AttributeError: 'int' object has no attribute 'read'At the from_param level it should really be a TypeError, but that's technically a non-backwards compatible change.
I raise an AttributeError, maybe can be changed for a TypeError next major version (or now if you don't mind the error type change)
There was a problem hiding this comment.
Thanks @Avasam, I am happy with either.
I don't think that changing to TypeError should be a big impact because it looks like a very edge-y case.
Probably the important part is to raise the exception, not the exact exception class. So I am OK with risking it.
But equally happy to take the conservative approach, given how susceptible setuptools is to Hyrum's Law...
There was a problem hiding this comment.
How about this: Keep it to AttributeError for this PR, I'll open a follow-up that changes to TypeError that you can merge before the next major update. That should cover our bases and improve the communicated error.
setuptools/command/easy_install.py
Outdated
| return cls.from_environment() | ||
| # otherwise, assume it's a string. | ||
| return cls.from_string(param) | ||
| return cls.from_environment() |
There was a problem hiding this comment.
Let's raise it explicitly because before cls.from_environment() was only used in the case of None. I suspect that the previous fallback to cls.from_string would cause a TypeError if the user passed non-sensical args.
We propbably don't want to change that behaviour
setuptools/extension.py
Outdated
| @@ -126,10 +127,12 @@ class Extension(_Extension): | |||
| specified on Windows. (since v63) | |||
| """ | |||
|
|
|||
| def __init__(self, name, sources, *args, **kw): | |||
| def __init__( | |||
| self, name: str, sources: list[str], *args, py_limited_api: bool = False, **kw | |||
There was a problem hiding this comment.
Jason might have added support for PathLikesources recently in pypa/distutils#237 (https://github.com/pypa/setuptools/blob/v72.2.0/setuptools/_distutils/extension.py#L29).
(PS.: granted they are incompatible with SETUPTOOLS_USE_DISTUTILS=stdlib but that is a deprecated code path).
There was a problem hiding this comment.
Thanks for the heads up. That added annotation isn't necessary for this PR. I'd rather avoid possible type conflicts with sdtlib distutils for now so I'll just remove the annotation to sources. I'll tackle that later, and happily bring in StrPath/PathLike updates from _distutils.
Avasam
force-pushed
the
runtime-changes-for-typeshed-merge
branch
from
4a161cd to
9c71fd8
Compare
Co-authored-by: Anderson Bravalheri <[email protected]>
Avasam
force-pushed
the
runtime-changes-for-typeshed-merge
branch
from
9c71fd8 to
9334056
Compare
|
Thank you! |
Summary of changes
Extracted from #4504
These are the runtime changes that should be reviewed more carefully.
Works towards #2345
Pull Request Checklist
newsfragments/.(See documentation for details)