Skip to content

Conversation

@sobolevn
Copy link
Member

@sobolevnsobolevn commented Feb 5, 2025

There was one error locally:

» mypy --config-file Tools/build/mypy.ini Tools/build/compute-changes.py:90: error: Incompatible return value type (got "frozenset[Path]", expected "set[Path]") [return-value] return frozenset(map(Path, filter(None, map(str.strip, changed_files)))) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Found 1 error in 1 file (checked 2 source files) 

Copy link
Member

@AA-TurnerAA-Turner left a comment

Choose a reason for hiding this comment

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

Please use Set rather than the concrete class.

A

@bedevere-app
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@@ -1,5 +1,5 @@
[mypy]
files = Tools/build/generate_sbom.py
files = Tools/build/generate_sbom.py, Tools/build/compute-changes.py
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this a multi-line list? Ideally we could use a TOML file, does MyPy support mypy.toml or .mypy.toml?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

It supports pyproject.toml, but it can affect stuff. + Our CI expects all folders to have mypy.ini. Current version of mypy does not support multiline strings here. But, I will fix that upstream. So, we can expect this in 1.16.0 somewhere.

Copy link
Member

Choose a reason for hiding this comment

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

Seems to work like this?

Suggested change
files = Tools/build/generate_sbom.py, Tools/build/compute-changes.py
files =
Tools/build/compute-changes.py,
Tools/build/generate_sbom.py

(Unfortunately can't add a trailing comma to the last entry.)

Copy link
Member

Choose a reason for hiding this comment

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

I've created a MyPy issue to request support for mypy.toml or similar: python/mypy#18617

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I will fix it shortly, sorry for the , bug 😢
I've fixed it several times already, but it is very persistent for some reason.

@sobolevn
Copy link
MemberAuthor

I don't quite agree about the Set change, since we work with concrete types in our conrete case, but I won't debate about it :)

sobolevnand others added 2 commits February 6, 2025 12:03
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@AA-Turner
Copy link
Member

I don't quite agree about the Set change, since we work with concrete types in our conrete case

True! My rationale is that it might make sense in some future change to use set internally rather than frozenset in some functions, but the only interface we promise is a non-mutable set. We could perhaps even change to Iterable[Path], as we only iterate over the collection at present.

@AA-TurnerAA-Turner merged commit 8b2fb62 into python:mainFeb 6, 2025
26 of 27 checks passed
sobolevn added a commit to python/mypy that referenced this pull request Feb 6, 2025
…i` (#18621) Now ```ini files = a.py, b.py ``` and ```ini files = a.py, b.py, ``` will be the same thing. Previously, adding a traling comma would add `''` to `paths`, which resulted in a strange error like: ``` a.py: error: Duplicate module named "a" (also at "a.py") (diff) a.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info (diff) a.py: note: Common resolutions include: a) using `--exclude` to avoid checking one of them, b) adding `__init__.py` somewhere, c) using `--explicit-package-bases` or adjusting MYPYPATH (diff) ``` Refs #14240 Refs python/cpython#129708
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Feb 7, 2025
…ython#129708) Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com> Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
cmaloney pushed a commit to cmaloney/cpython that referenced this pull request Feb 8, 2025
…ython#129708) Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com> Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
x612skm pushed a commit to x612skm/mypy-dev that referenced this pull request Feb 24, 2025
…i` (python#18621) Now ```ini files = a.py, b.py ``` and ```ini files = a.py, b.py, ``` will be the same thing. Previously, adding a traling comma would add `''` to `paths`, which resulted in a strange error like: ``` a.py: error: Duplicate module named "a" (also at "a.py") (diff) a.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info (diff) a.py: note: Common resolutions include: a) using `--exclude` to avoid checking one of them, b) adding `__init__.py` somewhere, c) using `--explicit-package-bases` or adjusting MYPYPATH (diff) ``` Refs python#14240 Refs python/cpython#129708
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@sobolevn@AA-Turner@hugovk