-
Notifications
You must be signed in to change notification settings - Fork 82
WIP: Fix most mypy issues #343
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
base: main
Are you sure you want to change the base?
Changes from all commits
ed56168
80a9eec
fd57c55
ab2d859
eaabc09
0712432
e3a5045
f78deb3
7e46ea7
9e029d7
5033f53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,10 +57,11 @@ def mkpath(name: pathlib.Path, mode=0o777, verbose=True, dry_run=False) -> None: | |
| if verbose and not name.is_dir(): | ||
| log.info("creating %s", name) | ||
|
|
||
| try: | ||
| dry_run or name.mkdir(mode=mode, parents=True, exist_ok=True) | ||
| except OSError as exc: | ||
| raise DistutilsFileError(f"could not create '{name}': {exc.args[-1]}") | ||
| if not dry_run: | ||
| try: | ||
| name.mkdir(mode=mode, parents=True, exist_ok=True) | ||
| except OSError as exc: | ||
| raise DistutilsFileError(f"could not create '{name}': {exc.args[-1]}") | ||
|
Comment on lines
+60
to
+64
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really don't love how this code gets more nested. What is it about mypy that's forcing different logic here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one could be ignored on the line itself without being unsafe. It's an unused expression, both mypy and pyright warn here because of the lack of assignement, it "looks like" a possible mistake because the result of the expression is not used for anything. But it's just a conditional execution pattern. It's definitely more of a lint than a type error. |
||
|
|
||
|
|
||
| @mkpath.register | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -249,6 +249,7 @@ def getopt(self, args: Sequence[str] | None = None, object=None): # noqa: C901 | |
| raise DistutilsArgError(msg) | ||
|
|
||
| for opt, val in opts: | ||
| value: int | str = val | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is gross.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, this may be resolved in mypy 1.18: https://mypy.readthedocs.io/en/stable/changelog.html#flexible-variable-definitions-update Before 1.18 the flag wasn't stabilized and called Edit: No it doesn't help here because of the difference of scope
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's this related request python/mypy#15988 As for casting, I could do # Unsafely casting to avoid using a different variable
# python/mypy#15988 could allow `val: str | int = val`
for opt, val in cast("list[tuple[str, int | str]]", opts): |
||
| if len(opt) == 2 and opt[0] == '-': # it's a short option | ||
| opt = self.short2long[opt[1]] | ||
| else: | ||
|
|
@@ -260,21 +261,21 @@ def getopt(self, args: Sequence[str] | None = None, object=None): # noqa: C901 | |
| opt = alias | ||
|
|
||
| if not self.takes_arg[opt]: # boolean option? | ||
| assert val == '', "boolean option can't have value" | ||
| assert value == '', "boolean option can't have value" | ||
| alias = self.negative_alias.get(opt) | ||
| if alias: | ||
| opt = alias | ||
| val = 0 | ||
| value = 0 | ||
| else: | ||
| val = 1 | ||
| value = 1 | ||
|
|
||
| attr = self.attr_name[opt] | ||
| # The only repeating option at the moment is 'verbose'. | ||
| # It has a negative option -q quiet, which should set verbose = False. | ||
| if val and self.repeat.get(attr) is not None: | ||
| val = getattr(object, attr, 0) + 1 | ||
| setattr(object, attr, val) | ||
| self.option_order.append((opt, val)) | ||
| if value and self.repeat.get(attr) is not None: | ||
| value = getattr(object, attr, 0) + 1 | ||
| setattr(object, attr, value) | ||
| self.option_order.append((opt, value)) | ||
|
|
||
| # for opts | ||
| if created_object: | ||
|
|
||
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.
Decision to be taken here.
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.
My instinct - get_command_obj should raise the error and always return Command... unless there's a case where None is legitimately expected, in which case this should be wrapped in a helper to raise that error and set the expectation.
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 just did a quick scan through the distutils and setuptools codebase and there's not a single call to
get_finalized_command(..., create=False). Maybe the create flag is just cruft that can be eliminated.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 opened an exploratory PR deprecating (unused, but not removed, for backward compatibility) the
createarg: #377