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

Update to mypy 0.991 for compatible-mypy & CI #1260

Merged
merged 5 commits into from
Jan 24, 2023
Merged

Conversation

intgr
Copy link
Collaborator

@intgr intgr commented Nov 15, 2022

Made a few tweaks that were needed to get our CI passing with mypy 0.991.

There is still a new regression with mypy_django_plugin that I haven't investigated, which happens with Django 3.2 only. Created an issue for it: #1261 Disabled testing with Django 2.2 and 3.2 in CI for now. Thanks to @flaeppe for the fix!

setup.py Outdated Show resolved Hide resolved
@intgr intgr force-pushed the update-to-mypy-0.991 branch from 8aaaee9 to 075caf5 Compare January 4, 2023 14:12
.github/workflows/test.yml Outdated Show resolved Hide resolved
@intgr intgr marked this pull request as ready for review January 4, 2023 15:09
@intgr intgr marked this pull request as draft January 23, 2023 10:38
@intgr intgr force-pushed the update-to-mypy-0.991 branch from 84993cf to 32f18cd Compare January 23, 2023 10:40
@intgr intgr force-pushed the update-to-mypy-0.991 branch from 3b8cbb6 to 9115891 Compare January 23, 2023 14:02
@intgr intgr marked this pull request as ready for review January 23, 2023 14:02
@intgr
Copy link
Collaborator Author

intgr commented Jan 23, 2023

Restored CI testing with Django 2.2 and 3.2.

This is now ready for review. Hoping to get this included in the next release (#1334). Not sure who has time to look at this, maybe @flaeppe or @ljodal?

warn_unused_ignores = True
warn_redundant_casts = True
warn_unused_configs = True
warn_unreachable = True
disallow_untyped_defs = true
disallow_incomplete_defs = true
show_error_codes = False
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

show_error_codes default changed in mypy 0.990.

I'm reverting to the old behavior, because so many .yml test files have mypy error messages without the error code.

Copy link
Member

Choose a reason for hiding this comment

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

Should we try to work towards show_error_codes = True? It feels reasonable that tests are granular enough to fail if a specific error code changes.

It could be an option to try to resolve this over time. Best thing would probably be to have a linter that disallows type: ignore i.e. without code in the test suite. I suppose that could be tracked in an issue.

warn_unused_ignores = True
warn_redundant_casts = True
warn_unused_configs = True
warn_unreachable = True
disallow_untyped_defs = true
disallow_incomplete_defs = true
show_error_codes = False
disable_error_code = empty-body
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replaced warn_no_return = False with disable_error_code = empty-body, the latter is less dangerous as it now only applies to empty functions.

@@ -309,6 +311,8 @@ def get_type_analyze_hook(self, fullname: str) -> Optional[Callable[[AnalyzeType
"django_stubs_ext.annotations.WithAnnotations",
):
return partial(handle_annotated_type, django_context=self.django_context)
else:
return None
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was added because I removed warn_no_return = False mypy setting.

@intgr
Copy link
Collaborator Author

intgr commented Jan 24, 2023

I really want this for the next release, but it has been difficult to get anyone to review this. Maybe @ngnpope?

Copy link
Member

@flaeppe flaeppe left a comment

Choose a reason for hiding this comment

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

I think this looks good, just had 1 comment regarding show_error_codes

warn_unused_ignores = True
warn_redundant_casts = True
warn_unused_configs = True
warn_unreachable = True
disallow_untyped_defs = true
disallow_incomplete_defs = true
show_error_codes = False
Copy link
Member

Choose a reason for hiding this comment

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

Should we try to work towards show_error_codes = True? It feels reasonable that tests are granular enough to fail if a specific error code changes.

It could be an option to try to resolve this over time. Best thing would probably be to have a linter that disallows type: ignore i.e. without code in the test suite. I suppose that could be tracked in an issue.

@intgr
Copy link
Collaborator Author

intgr commented Jan 24, 2023

Awesome, thanks!

Should we try to work towards show_error_codes = True? It feels reasonable that tests are granular enough to fail if a specific error code changes.

For adding type: ignore comments it would be helpful.

But in the .yml test suite, we already compare the full error messages, which is specific enough. I doubt mypy will ever have two different error codes with the same message.

It would make the already long # E: ... lines even longer. It's also a bit finicky because the error code is separated by two spaces (might not be obvious that a diff is between one and two spaces).

@intgr intgr merged commit 9874789 into master Jan 24, 2023
@intgr intgr deleted the update-to-mypy-0.991 branch January 24, 2023 09:36
@intgr intgr mentioned this pull request Jan 24, 2023
@flaeppe
Copy link
Member

flaeppe commented Jan 24, 2023

Awesome, thanks!

Should we try to work towards show_error_codes = True? It feels reasonable that tests are granular enough to fail if a specific error code changes.

For adding type: ignore comments it would be helpful.

But in the .yml test suite, we already compare the full error messages, which is specific enough. I doubt mypy will ever have two different error codes with the same message.

It would make the already long # E: ... lines even longer. It's also a bit finicky because the error code is separated by two spaces (might not be obvious that a diff is between one and two spaces).

Ah, right, didn't think about that it was the reverse case that was the issue. I suppose if we'd prefer matching on code instead of a complete message one could go with # ER: .*msg.*[some-code] (regex match). But I agree, it's good enough matching the message, like the suite does now.

@ngnpope
Copy link
Contributor

ngnpope commented Jan 24, 2023

I really want this for the next release, but it has been difficult to get anyone to review this. Maybe @ngnpope?

Sorry, I do want to help out, but I have to squeeze it in around everything else... 🙂 Me right now: 🤹🏻

I suppose if we'd prefer matching on code instead of a complete message one could go with # ER: .msg.[some-code] (regex match).

I don't think that would work so well in most cases because the messages are often formatted with the type values which we then would not detect changes for...

@intgr
Copy link
Collaborator Author

intgr commented Jan 24, 2023

Sorry, I do want to help out, but I have to squeeze it in around everything else...

No worries :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants