Skip to content

Conversation

@sobolevn
Copy link
Member

Let's see how it goes.

@sobolevn
Copy link
Member Author

There are tons of new errors due to new checks of "untyped code". I will see how easy it would be to fix them.

@sobolevn
Copy link
Member Author

Well, there are so many errors that I start to think that it might be a regression in mypy: https://github.com/python/typeshed/actions/runs/10187104121/job/28180408403?pr=12463 cc @hauntsaninja

@github-actions

This comment has been minimized.

srittau
srittau previously approved these changes Aug 1, 2024
@srittau srittau dismissed their stale review August 1, 2024 06:53

Missed the ci failures

@AlexWaygood
Copy link
Member

AlexWaygood commented Aug 1, 2024

There are also many type: ignore comments that we can get rid of with this release, but they don't show up in CI: see #12178 (comment). Remember that --warn-unused-ignore doesn't work in typeshed directories out of the box.

@AlexWaygood
Copy link
Member

AlexWaygood commented Aug 1, 2024

The changes to the overload-overlap error code are a result of python/mypy#17392. Overall we should be able to get rid of more type: ignore[overload-overlap] comments than we have to add as a result of that change.

The new override errors are a result of python/mypy#17276.

At first glance I think everything is working as expected here; I doubt there's a mypy regression to report

@srittau
Copy link
Collaborator

srittau commented Aug 1, 2024

We could do #12178 now if that makes transition easier. I don't think it's critical to do it after the mypy update.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

sobolevn commented Aug 3, 2024

@srittau @AlexWaygood any ideas on why stubtest ignores --mypy-config-file option with [mypy]\ndisable_error_code = overload-overlap inside?

@github-actions

This comment has been minimized.

@Daverball
Copy link
Contributor

Daverball commented Aug 3, 2024

@srittau @AlexWaygood any ideas on why stubtest ignores --mypy-config-file option with [mypy]\ndisable_error_code = overload-overlap inside?

Maybe it's because it doesn't have a file ending? So it doesn't know whether it should use the INI or TOML parser, so it doesn't use either and the file is just silently ignored. Is there any reason not to put a [tool.mypy] section into the pyproject.toml for the part of the configuration that's the same for every mypy/stubtest run and use that instead of a common set of static command line arguments and generating a temporary config file for stubtest? I feel like that would help with setting up contributor's development environments to more closely match what the CI will check. So they don't manually have to match their editor/IDE's configuration, since the pyproject.toml will generally be auto-discovered by tools.

Edit: It also makes it easier to discover what checks are enabled/disabled by default in typeshed.

@AlexWaygood
Copy link
Member

Is there any reason not to put a [tool.mypy] section into the pyproject.toml for the part of the configuration that's the same for every mypy/stubtest run and use that instead of a common set of static command line arguments and generating a temporary config file for stubtest?

@Daverball we've discussed that idea previously in #2852. I think it would honestly just make our config situation more complicated, because different tests are still going to require slightly different options. It'll be hard to tell which options are provided by the global config and which are overridden in the specific test script, and it could mislead contributors into thinking that running mypy on typeshed is simpler than it is.

@sobolevn
Copy link
Member Author

sobolevn commented Aug 3, 2024

Nope, explicit extension didn't work:

Command run was: /opt/hostedtoolcache/Python/3.8.18/x64/bin/python -m mypy.stubtest --check-typeshed --custom-typeshed-dir . --allowlist stdlib/@tests/stubtest_allowlists/common.txt --allowlist stdlib/@tests/stubtest_allowlists/linux.txt --allowlist stdlib/@tests/stubtest_allowlists/py38.txt --allowlist stdlib/@tests/stubtest_allowlists/linux-py38.txt --mypy-config-file /tmp/tmpoc4g5odk.ini --ignore-positional-only

Looks like I will be debugging this manually now :)

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

sobolevn commented Aug 3, 2024

This is a mypy bug :)
I will report and fix it today.

@sobolevn
Copy link
Member Author

sobolevn commented Aug 3, 2024

If anyone is interested in reviewing my PR it is here: python/mypy#17629

@sobolevn
Copy link
Member Author

sobolevn commented Aug 4, 2024

I dropped all config changes, because it is not yet supported by stubtest (until next release).

@github-actions

This comment has been minimized.

@sobolevn sobolevn marked this pull request as draft August 4, 2024 12:07
@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

sobolevn commented Aug 4, 2024

Ok, the first part is done. Now I will run tests with --warn-unused-ignores and remove unused ignores (there might be many of them).

@github-actions

This comment has been minimized.

@sobolevn sobolevn marked this pull request as ready for review August 4, 2024 14:25
@sobolevn
Copy link
Member Author

sobolevn commented Aug 4, 2024

@srittau @AlexWaygood done! It took me quite some time to finish, but now it is ready! 🎉

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2024

Diff from mypy_primer, showing the effect of this PR on open source code:

bokeh (https://github.com/bokeh/bokeh)
- src/bokeh/models/util/structure.py:165: note: ... from here:

cwltool (https://github.com/common-workflow-language/cwltool)
- note: ... from here,
- note: ... from here,
- cwltool/utils.py:52: note: ... from here:

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Nice, thank you! Great that this catches a bug in reportlab source too :-)

@hauntsaninja hauntsaninja merged commit 6f248df into main Aug 5, 2024
@hauntsaninja hauntsaninja deleted the sobolevn-patch-2 branch August 5, 2024 06:19
max-muoto pushed a commit to max-muoto/typeshed that referenced this pull request Aug 17, 2024
max-muoto pushed a commit to max-muoto/typeshed that referenced this pull request Sep 8, 2024
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.

6 participants