-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Merge typeshed return annotations #4744
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 typeshed return annotations #4744
Conversation
fb5487b
to
aa0ad57
Compare
# We shouldn't find any non-pyc files in __pycache__ | ||
assert m is not None |
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.
Not strictly an annotation change, but I included it here because it didn't feel worth doing a PR just for this
Would've been included in #4753 if I realised sooner this was an easy fix
setuptools/command/bdist_wheel.py
Outdated
def run(self) -> None: | ||
def run(self): |
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 opted out of annotating this specific run
in this PR for the same reason I didn't do initialize_options
.
(note that I'm commenting on a specific commit, this PR doesn't remove any return annotation)
8cba3e1
to
5a26893
Compare
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.
Thank you very much Avasam.
I have some minor comments that I missed last time.
setuptools/command/develop.py
Outdated
@@ -27,18 +28,20 @@ class develop(Command): | |||
prefix = None | |||
index_url = None | |||
|
|||
def run(self): | |||
cmd = ( | |||
def run(self) -> None: # type: ignore[override] # Not including easy_install's show_deprecation argument |
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 am trying to understand this comment.
Maybe it is also a left over from merge/rebase from a time that develop
extended easy_install
before the recent removal in #4955?
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.
Indeed, this type-ignore no longer applies. This used to break the Liskov Substitution Principle.
I think we're not currently checking for unused type-ignores because of some inconsistent error codes with Python 3.8 and/or distutils.
Thanks for the reviews @abravalheri ! Only a few steps left until setuptool's public interface is on-par with TypeShed's stubs. Whilst benefiting from type-checking internally. |
Co-authored-by: Anderson Bravalheri <[email protected]>
Summary of changes
Based on #4736, so merge that one first.DoneBlocked by setuptools/msvc.py dir methods and properties split off to #4755 for easier reviewDoneStep 6.2 of #2345 (comment)
This PR still doesn't do
initialize_options
as those have their own set of issues (and knowing they returnNone
isn't that helpful to begin with), as well as complex Command-based overloads.Pull Request Checklist
newsfragments/
. (not public facing yet)(See documentation for details)