-
-
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
Runtime typing fixes for typeshed return type merge #4753
Runtime typing fixes for typeshed return type merge #4753
Conversation
@@ -220,7 +220,7 @@ class bdist_wheel(Command): | |||
|
|||
def initialize_options(self) -> None: | |||
self.bdist_dir: str | None = None | |||
self.data_dir: str | None = None | |||
self.data_dir = "" |
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.
data_dir
is always set in finalize_options
, so if I understand correctly this value doesn't matter and it'll always be a string.
@@ -90,10 +89,7 @@ def __getattr__(self, attr: str): | |||
return orig.build_py.__getattr__(self, attr) | |||
|
|||
def build_module(self, module, module_file, package): | |||
outfile, copied = orig.build_py.build_module(self, module, module_file, package) | |||
if copied: | |||
self.__updated_files.append(outfile) |
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.
Looks like __updated_files
was completely unused, and its mangled
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 see, apparently it was introduced for the 2to3
transformation: 5b56888.
In this case, does it make sense to keep this method? (It will only call super()
right?)
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.
You're right! I didn't even notice that. I'd remove the method entirely at this point.
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 agree, it is better to remove it.
f7445e7
to
593bf67
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.
@@ -90,10 +89,7 @@ def __getattr__(self, attr: str): | |||
return orig.build_py.__getattr__(self, attr) | |||
|
|||
def build_module(self, module, module_file, package): | |||
outfile, copied = orig.build_py.build_module(self, module, module_file, package) | |||
if copied: | |||
self.__updated_files.append(outfile) |
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 see, apparently it was introduced for the 2to3
transformation: 5b56888.
In this case, does it make sense to keep this method? (It will only call super()
right?)
) -> None: | ||
"""Same idea as :func:`_read_utf8_with_fallback`, but for the | ||
:meth:`ConfigParser.read` method. | ||
:meth:`RawConfigParser.read` method. |
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.
This also allows any subclass, right?
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.
Correct. Setuptools passes a RawConfigParser
to this method, but ConfigParser
, which is a subclass, will be statically allowed.
593bf67
to
9a4c8d4
Compare
Summary of changes
Runtime typing fixes for #4744 that shouldn't incur any functional change for the end-users.
Pull Request Checklist
newsfragments/
. (not public-facing)(See documentation for details)