fix: minor touchups based on stricter mypy settings #1060
Conversation
| @@ -30,7 +30,8 @@ repos: | |||
| rev: v1.2.5 | |||
| hooks: | |||
| - id: pycln | |||
| args: [--config=pyproject.toml] | |||
| args: [--all] | |||
| stages: [manual] | |||
There was a problem hiding this comment.
Moving this to manual, since it's still very handy, but can get in the way.
| - id: python-check-blanket-type-ignore | ||
| stages: [manual] |
There was a problem hiding this comment.
In MyPy 0.940, this is now an option, and using MyPy, it not only tells you that you need an error code, it suggests the correct one! :)
| @@ -96,7 +96,7 @@ def architectures(self) -> Set[Architecture]: | |||
| return self.globals.architectures | |||
|
|
|||
|
|
|||
| Setting = Union[Dict[str, str], List[str], str] | |||
| Setting = Union[Dict[str, str], List[str], str, int] | |||
There was a problem hiding this comment.
Discovered by the unreachable check.
| if config_value is None: | ||
| if not config_value: |
There was a problem hiding this comment.
@mayeut The original version should be impossible to reach - self.reader.get should always return strings, not None, right? If it can return None, then we need some significant fixes to the typing in other places.
Discovered by unreachability.
There was a problem hiding this comment.
(Going to mark this draft until verified)
There was a problem hiding this comment.
The original version should be impossible to reach
Seems right. Not sure why it wasn't checked like manylinux here.
There was a problem hiding this comment.
There's another difference between musllinux & manylinux.
manylinux has:
if not config_value:
# default to manylinux2014
image = pinned_images.get("manylinux2014")
elif ....
...
assert image is not None
while musllinux has (after the change):
if not config_value:
image = pinned_images["musllinux_1_1"]
elif ....
....
Not sure why get is used for manylinux (which is the only reason for assert image is not None). It would be nicer to use image = pinned_images["manylinux2014"] no ?
I've not been through the entire history so there might be some historic reasons for this being written this way but might not make sense anymore.
There was a problem hiding this comment.
I'd guess it's a historical thing, perhaps from the days that manylinux2010 was the default on some platforms and manylinux2014 on others.
It would be nicer to use
image = pinned_images["manylinux2014"]no ?
Agreed.
| if config_value is None: | ||
| if not config_value: |
There was a problem hiding this comment.
I'd guess it's a historical thing, perhaps from the days that manylinux2010 was the default on some platforms and manylinux2014 on others.
It would be nicer to use
image = pinned_images["manylinux2014"]no ?
Agreed.
| @@ -96,7 +96,7 @@ def architectures(self) -> Set[Architecture]: | |||
| return self.globals.architectures | |||
|
|
|||
|
|
|||
| Setting = Union[Dict[str, str], List[str], str] | |||
| Setting = Union[Dict[str, str], List[str], str, int] | |||
henryiii
force-pushed
the
henryiii/fix/mypyup
branch
from
38f0925 to
5143839
Compare
henryiii
added
automerge
|
Actually, I'll leave auto merge off, so we can merge all three ready PRs in a row to reduce GHA usage. |
Using some stricter (and at least one new in 0.940) setting for mypy.