-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge simple type annotations from typeshed #4504
base: main
Are you sure you want to change the base?
Conversation
setuptools/extension.py
Outdated
def __init__( | ||
self, name: str, sources: list[str], *args, py_limited_api: bool = False, **kw | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From @abravalheri https://github.com/pypa/setuptools/pull/4505/files#r1709465928
Jason might have added support for
PathLike
sources recently in pypa/distutils#237 (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).
setuptools/build_meta.py
Outdated
@@ -289,7 +297,7 @@ def _arbitrary_args(self, config_settings: _ConfigSettings) -> Iterator[str]: | |||
|
|||
|
|||
class _BuildMetaBackend(_ConfigSettingsTranslator): | |||
def _get_build_requires(self, config_settings, requirements): | |||
def _get_build_requires(self, config_settings: _ConfigSettings, requirements): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is likely here that requirements
and the return value are both list[str]
. Not sure how the type-checking would react to that though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't cause any issue. I didn't add annotations to _get_build_requires
because it's not typed in typeshed. config_settings
param being typed is incidental to adding all _ConfigSettings
.
But since you're mentioning it here and it's not an issue, I've added the type annotation to requirements
param
@abravalheri I'll answer a couple comments at once:
|
if TYPE_CHECKING: | ||
from setuptools import Distribution | ||
from typing_extensions import TypeAlias | ||
|
||
StrIter: TypeAlias = Iterator[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel like this alias brings anything of value:
- It doesn't work around any runtime issue
- It doesn't abstract away any complexity
- It's not clearer than the type it's aliasing, or doesn't help explain it
- It's barely any more concise (because it's using a shorthand instead of being clear and explicit)
In fact, it might actually hurt understandability as a dev can reasonably assume that the alias might be abstracting a more complex type and have to check what it is, rather than immediately understanding it's Iterator[str]
But it's not private. Could we maybe deprecate it ?
8f34df1
to
f319cc5
Compare
f319cc5
to
d4143c0
Compare
9e0cc8d
to
3113380
Compare
3113380
to
4eef53a
Compare
…ools-simple-typeshed-params
…ools-simple-typeshed-params
…ools-simple-typeshed-params
…ools-simple-typeshed-params
Summary of changes
Please review and merge first: #4505DoneMerging #4581 and #4584 first will reduce changes in this PRDoneIt's important to validate this PR with mypy, let's re-enable it first and fix existing failures: #4604
Not done in this PR:
@overload
: Merge overloaded method definitions from typeshed #4506AbstractSandbox
's dynamically created methods@type_check_only
, and other TypedDict from typeshedStep 6.1 of #2345 (comment)
Pull Request Checklist
newsfragments/
. (nothing user-facing as static typing users would still be usingtypes-setuptools
)(See documentation for details)