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

Migrate mypy config to pyproject.toml #3936

Merged
merged 9 commits into from Oct 16, 2023
Merged

Migrate mypy config to pyproject.toml #3936

merged 9 commits into from Oct 16, 2023

Conversation

acharles7
Copy link
Contributor

@acharles7 acharles7 commented Oct 9, 2023 •
edited

Description

This PR is focused on streamlining the project's configuration files. Instead of maintaining separate mypy.ini file, this PR help us consolidate remaining mypy configuration file within the pyproject.toml file as per PEP 518. This modification leads to a more concise and organized config files.

Relevant PR #3632 (Closed)

Checklist

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@JelleZijlstra JelleZijlstra added the skip news Pull requests that don't need a changelog entry. label Oct 9, 2023
JelleZijlstra
JelleZijlstra reviewed Oct 10, 2023
View reviewed changes
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks!

src/blackd/__init__.py
@@ -74,7 +74,7 @@ def main(bind_host: str, bind_port: int) -> None:
app = make_app()
ver = black.__version__
black.out(f"blackd version {ver} listening on {bind_host} port {bind_port}")
web.run_app(app, host=bind_host, port=bind_port, handle_signals=True, print=None)
web.run_app(app, host=bind_host, port=bind_port, handle_signals=True, print=None) # type: ignore[arg-type]
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the issue here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

print argument accepts Callable[..., None] = print
i.e.

src/blackd/__init__.py:77:81: error: Argument "print" to "run_app" has incompatible type "None"; expected "Callable[..., None]"  [arg-type]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like this is fixed on master: https://github.com/aio-libs/aiohttp/blob/30850babb43a8e28dd2df036776c62fd613d3d89/aiohttp/web.py#L453C5-L453C5. Can you add a comment saying that aiohttp had an incorrect annotation and that it will be fixed once aiohttp releases that code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, thanks

pyproject.toml Outdated
warn_return_any = true
# Unreachable blocks have been an issue when compiling mypyc, let's try to avoid 'em in the first place.
warn_unreachable = true
disallow_untyped_calls = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Many of these are included in --strict:

  --strict                  Strict mode; enables the following flags: --warn-unused-configs, --disallow-any-generics, --disallow-subclassing-any, --disallow-untyped-calls, --disallow-untyped-defs,
                            --disallow-incomplete-defs, --check-untyped-defs, --disallow-untyped-decorators, --warn-redundant-casts, --warn-unused-ignores, --warn-return-any, --no-implicit-
                            reexport, --strict-equality, --extra-checks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, They are now removed, Thanks

hauntsaninja
hauntsaninja reviewed Oct 10, 2023
View reviewed changes
pyproject.toml Outdated
# CI only checks src/, but in case users are running LSP or similar we explicitly ignore
# errors in test data files.
[[tool.mypy.overrides]]
module = ["tests.data.*", "scripts.*"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't ignore errors in scripts. It's fine to have a looser config apply to scripts though, like ignore missing imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I thought about it but then skipped as it'll add few extra ignores in repo.

  • Added type: ignore where necessary in scripts.*
  • Added types-commonmark, urllib3, hypothesmith as additional_dependencies in mypy hook to avoid adding type: ignore (let me know if its not the right way)

@acharles7
Copy link
Contributor Author

@JelleZijlstra @hauntsaninja Appreciate any tip you can provide for resolving these failed checks!

JelleZijlstra
JelleZijlstra reviewed Oct 11, 2023
View reviewed changes
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

The CI failures look like mypyc isn't picking up the ignore_missing_imports in the config file. Not sure why that is, but possibly the hatch-mypyc config needs to be tweaked.

src/blackd/__init__.py
@@ -74,7 +74,7 @@ def main(bind_host: str, bind_port: int) -> None:
app = make_app()
ver = black.__version__
black.out(f"blackd version {ver} listening on {bind_host} port {bind_port}")
web.run_app(app, host=bind_host, port=bind_port, handle_signals=True, print=None)
web.run_app(app, host=bind_host, port=bind_port, handle_signals=True, print=None) # type: ignore[arg-type]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like this is fixed on master: https://github.com/aio-libs/aiohttp/blob/30850babb43a8e28dd2df036776c62fd613d3d89/aiohttp/web.py#L453C5-L453C5. Can you add a comment saying that aiohttp had an incorrect annotation and that it will be fixed once aiohttp releases that code?

@github-actions
Copy link

diff-shades reports zero changes comparing this PR (41a5cd9) to main (935f303).

What is this? | Workflow run | diff-shades documentation

@acharles7
Copy link
Contributor Author

@JelleZijlstra @hauntsaninja Appreciate any tip you can provide for resolving these failed checks!

I looked into it and seems like adding

[tool.hatch.build.targets.wheel.hooks.mypyc]
mypy-args = ["--ignore-missing-imports"]

https://github.com/ofek/hatch-mypyc#mypy-arguments fixes the pipeline. However, I feel like its wildcard option and can ignore all imports errors. But, this makes CICD happy.
Moreover, Lint check will work as expected using mypy config from pyproject.toml and can catch imports errors if any.

I'm not sure why hatch mypyc hook is not using default pyproject.toml, maybe hatch mypyc limitation?

If you have any better way, Please suggest, Thanks!

@JelleZijlstra JelleZijlstra merged commit 6f84f65 into psf:main Oct 16, 2023
37 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@hauntsaninja hauntsaninja Awaiting requested review from hauntsaninja

Assignees
No one assigned
Labels
skip news Pull requests that don't need a changelog entry.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@acharles7 @JelleZijlstra @hauntsaninja