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

gh-129707: Check Tools/build/compute-changes.py with mypy #129708

Merged
merged 4 commits into from
Feb 6, 2025

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn 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-Turner AA-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

bedevere-app bot commented Feb 5, 2025

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
Member Author

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
Member Author

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
Member Author

sobolevn commented Feb 6, 2025

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 :)

@sobolevn sobolevn requested a review from AA-Turner February 6, 2025 09:03
sobolevn and others added 2 commits February 6, 2025 12:03
@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-Turner AA-Turner merged commit 8b2fb62 into python:main Feb 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
cmaloney pushed a commit to cmaloney/cpython that referenced this pull request Feb 8, 2025
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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants