Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: minor touchups based on stricter mypy settings #1060

Merged
merged 2 commits into from
Apr 2, 2022
Merged

fix: minor touchups based on stricter mypy settings #1060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6885e91
fix: minor touchups based on stricter mypy settings
henryiii Mar 22, 2022
5143839
refactor: address review comments
henryiii Apr 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 4 additions & 6 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ repos:
hooks:
- id: pycln
args: [--all]
additional_dependencies: [click<8.1] # temporary workaround until typer updates
stages: [manual]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this to manual, since it's still very handy, but can get in the way.


- repo: https://github.com/PyCQA/isort
rev: 5.10.1
Expand All @@ -54,7 +54,7 @@ repos:
- id: mypy
name: mypy 3.6 on cibuildwheel/
exclude: ^(bin|cibuildwheel/resources|docs)/.*py$
args: ["--python-version=3.6", "--show-error-codes"]
args: ["--python-version=3.6"]
additional_dependencies: &mypy-dependencies
- nox
- packaging>=21.0
Expand All @@ -72,11 +72,11 @@ repos:
- id: mypy
name: mypy 3.7+ on bin/
files: ^((bin|docs)/.*py)$
args: ["--python-version=3.7", "--show-error-codes"]
args: ["--python-version=3.7"]
additional_dependencies: *mypy-dependencies
- id: mypy
name: mypy 3.10
args: ["--python-version=3.10", "--show-error-codes"]
args: ["--python-version=3.10"]
additional_dependencies: *mypy-dependencies

- repo: https://github.com/asottile/yesqa
Expand All @@ -98,8 +98,6 @@ repos:
hooks:
- id: python-check-blanket-noqa
stages: [manual]
- id: python-check-blanket-type-ignore
stages: [manual]
Comment on lines -101 to -102
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! :)

- id: python-no-log-warn
- id: python-no-eval
- id: python-use-type-annotations
Expand Down
7 changes: 3 additions & 4 deletions cibuildwheel/options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discovered by the unreachable check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.



class Override(NamedTuple):
Expand Down Expand Up @@ -482,21 +482,20 @@ def build_options(self, identifier: Optional[str]) -> BuildOptions:

if not config_value:
# default to manylinux2014
image = pinned_images.get("manylinux2014")
image = pinned_images["manylinux2014"]
elif config_value in pinned_images:
image = pinned_images[config_value]
else:
image = config_value

assert image is not None
manylinux_images[build_platform] = image

for build_platform in MUSLLINUX_ARCHS:
pinned_images = all_pinned_docker_images[build_platform]

config_value = self.reader.get(f"musllinux-{build_platform}-image")

if config_value is None:
if not config_value:
Comment on lines -499 to +498
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Going to mark this draft until verified)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original version should be impossible to reach

Seems right. Not sure why it wasn't checked like manylinux here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image = pinned_images["musllinux_1_1"]
elif config_value in pinned_images:
image = pinned_images[config_value]
Expand Down
5 changes: 5 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ files = [
"noxfile.py",
]
warn_unused_configs = true
show_error_codes = true

warn_redundant_casts = true
no_implicit_reexport = true
strict_equality = true
Expand All @@ -49,6 +51,9 @@ disallow_any_generics = true
warn_return_any = true
no_implicit_optional = true

enable_error_code = ["ignore-without-code", "redundant-expr", "truthy-bool"]
warn_unreachable = true

[[tool.mypy.overrides]]
module = "cibuildwheel.*"
disallow_untyped_defs = true
Expand Down
2 changes: 1 addition & 1 deletion unit_test/wheel_print_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def test_no_printout_on_error(tmp_path, capsys):
tmp_path.joinpath("example.1").touch()
raise RuntimeError()

captured = capsys.readouterr()
captured = capsys.readouterr() # type: ignore[unreachable]
assert captured.err == ""

assert "example.0" not in captured.out
Expand Down