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 1.10 #506

Merged
merged 2 commits into from
Jun 4, 2024
Merged

Update to mypy 1.10 #506

merged 2 commits into from
Jun 4, 2024

Conversation

amotl
Copy link
Member

@amotl amotl commented Apr 10, 2024

About

Update to the most recent version of mypy, in order to support the update to Python 3.11 on a subsequent iteration.

@amotl amotl force-pushed the update-mypy branch 3 times, most recently from 3c2dd03 to 0541502 Compare April 10, 2024 15:39
@amotl amotl marked this pull request as ready for review April 10, 2024 15:41
Copy link
Contributor

@plaharanne plaharanne left a comment

Choose a reason for hiding this comment

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

It should be changed in .github/dependabot.yml too

@@ -138,7 +139,7 @@ def regions_delete(args: Namespace) -> None:
print_response(
data=data,
errors={
"message": region_errors.get("message"),
"message": cast(str, region_errors.get("message")),
Copy link
Member Author

Choose a reason for hiding this comment

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

That might be a viable alternative instead of using a cast. wdyt?

Suggested change
"message": cast(str, region_errors.get("message")),
"message": region_errors.get("message", "unknown error"),

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it makes sense to have a meaningful message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi. I changed the code according to the suggestion. The patch is now ready for review, if you don't have any other objections.

Copy link
Member Author

Choose a reason for hiding this comment

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

Bumping the mypy version should be done in .github/dependabot.yml too.

Right. Done.

self.server.on_token(token)
self.server.on_token(token) # type: ignore[attr-defined]
Copy link
Member Author

Choose a reason for hiding this comment

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

I've marked this attribute to be ignored. Apparently, the on_token is a non-standard extension, where the type checker trips on, but in reality, all just works well?

Comment on lines +1 to +6
[tool.mypy]
check_untyped_defs = true
ignore_missing_imports = true
implicit_optional = true
install_types = true
non_interactive = true
Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I used the chance to introduce a pyproject.toml, which can absorb more project metadata in the future while we go. I hope you agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good. Fabian is working on migrating our projects to Poetry, I guess he would eventually create it anyway.

Copy link
Member Author

@amotl amotl Apr 30, 2024

Choose a reason for hiding this comment

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

Is your decision on Poetry already fixed? I think classic setuptools/pip is mature enough today so you don't exactly need Poetry, mostly. Also, it is aging a bit.

We are only using it on Wetterdienst today, and abstained from bringing it into many other projects so far, because, well, we didn't actually see an advantage given that vanilla pip/setuptools supports pyproject.toml configurations well, and it certainly adds a bit of drag as well.

/cc @Taliik

Comment on lines +20 to +21
# type: ignore

Copy link
Member Author

Choose a reason for hiding this comment

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

On test_printer.py, mypy emits many type errors, so I figured it would be good to ignore them altogether. Let me know if you think different, then it will probably be a bigger story.

@amotl amotl force-pushed the update-mypy branch 3 times, most recently from bb3ec72 to 8820ed2 Compare May 21, 2024 16:51
@amotl amotl changed the title Update to mypy 1.9.0 Update to mypy 1.10 May 21, 2024
@amotl
Copy link
Member Author

amotl commented Jun 3, 2024

Hi. I've updated this PR according to corresponding suggestions from code review. Can you have a final look?

@amotl amotl merged commit f3c6744 into master Jun 4, 2024
10 checks passed
@amotl amotl deleted the update-mypy branch June 4, 2024 08:59
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.

2 participants