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.

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

PB-1056 Tighten type checking rules #28

Merged
merged 3 commits into from
Oct 10, 2024
Merged

Conversation

msom
Copy link
Contributor

@msom msom commented Oct 9, 2024

The type checker is more useful when more checks are activated, as it allow to to checking for undefined attributes or missing overloads, for example.

I suggest that we enable some additional checks:

  • enforce type definitions on methods (disallow_untyped_defs, disallow_incomplete_defs, disallow_untyped_decorators)
  • check types within untyped methods (check_untyped_defs)
  • show additional warnings for unused imports, missing return statements and unreachable code (warn_unused_ignores, warn_no_return, warn_unreachable)
  • restrict equality checks to same types (strict_equality)

But only for our own code (ignore_missing_imports) and not for tests (exclude).

@asteiner-swisstopo
Copy link
Contributor

General comment: Wouldn't it be easier to just set the strict mode and maybe disable things we absolutely don't want?

I had a go at this in branch sandbox-PB-1056-mypy-in-strict-mode (derived from this branch) and I had to adapt only very sensible things imho:
https://github.com/geoadmin/service-control/compare/feat-pb-1056-tighten-mypy..sandbox-PB-1056-mypy-in-strict-mode

Explicitly listing all the rules we want has the risk that mypy adds checks in the future and we don't add them here. Going with the --strict we are sure to always have them.

@msom
Copy link
Contributor Author

msom commented Oct 10, 2024

Good idea to use strict per default and relax specific rules, but I would use strict in the config file, rather than the CLI as the config file will also be considered in vscode.

@msom
Copy link
Contributor Author

msom commented Oct 10, 2024

@asteiner-swisstopo it's now as strict as your branch. I didn't use the monkeypatch part, as this results in a linting error. I think it's better to use a type:ignore here.

Copy link
Contributor

@schtibe schtibe left a comment

Choose a reason for hiding this comment

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

Very nice, just a small question!

mypy.ini Outdated Show resolved Hide resolved
Copy link
Contributor

@asteiner-swisstopo asteiner-swisstopo left a comment

Choose a reason for hiding this comment

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

Looks already pretty good, found some more opportunities to rely more on the default settings on strict mode.

mypy.ini Outdated Show resolved Hide resolved
mypy.ini Outdated Show resolved Hide resolved
mypy.ini Outdated Show resolved Hide resolved
mypy.ini Show resolved Hide resolved
@msom msom changed the title Tighten Some Type Checking Rules PB-1056 Tighten Type Checking Rules Oct 10, 2024
@msom msom changed the title PB-1056 Tighten Type Checking Rules PB-1056 Tighten type checking rules Oct 10, 2024
@msom msom merged commit 60f04a7 into develop Oct 10, 2024
7 checks passed
@msom msom deleted the feat-pb-1056-tighten-mypy branch October 10, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants