Skip to content

Test and fix trailing commas in many multiline string options in pyproject.toml #18624

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

Merged
merged 6 commits into from
Feb 7, 2025

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Feb 6, 2025

Refs #18621
Closes #18623

With a lot more tests.

This comment has been minimized.

@sobolevn
Copy link
Member Author

sobolevn commented Feb 6, 2025

I have no idea why this test fails on Win, I don't have an access to Win machine, so I just skipped this test on this OS.

______________________ testPyprojectPluginsTrailingComma ______________________
[gw3] win32 -- Python 3.9.13 D:\a\mypy\mypy\.tox\py39\Scripts\python.EXE
data: D:\a\mypy\mypy\test-data\unit\cmdline.pyproject.test:154:
Failed: Invalid output (D:\a\mypy\mypy\test-data\unit\cmdline.pyproject.test, line 154)
---------------------------- Captured stderr call -----------------------------
Expected:
Actual:
  pyproject.toml: Unescaped '/' in a string (at line 3, column 7) (diff)
  == Return code: 0 (diff)

Update the test output using --update-data (implies -n0; you can additionally use the -k selector to update only specific tests)

This comment has been minimized.

This comment has been minimized.

@sterliakov
Copy link
Collaborator

I also don't have anything windows to test and know very little about it, but that's understandable:

  • mypy/test/data.py replaces <ROOT> with root_dir (expand_variables) that is os.path.normpath of something. normpath does the bad thing (tm):

    On Windows, it converts forward slashes to backward slashes.

  • Now the file looks like

    [tool.mypy]
    plugins = """
       som\ething/test-data/...
    """
  • Now the toml parser chokes on it, because \ escape is invalid (in my example that's tomli.TOMLDecodeError: Unescaped '\' in a string (at line 3, column 9) - note the wording is exactly the same)

  • Finally, something (I don't see what exactly) erroneously "normalizes" \ to / in the error message, producing this incomprehensible output.

I confess I don't know how to fix this properly in a cross-platform way, and neither I know why testFunctionPluginFullnameIsNotNonePyProjectTOML (and other pyproject.toml-based tests near it) passes on windows.

@sterliakov
Copy link
Collaborator

Ah, I probably know: could you try just moving this test to check-custom-plugin.test and removing -posix? cmdline and check test groups seem to treat their temporary directories slightly differently

This comment has been minimized.

@sobolevn
Copy link
Member Author

sobolevn commented Feb 7, 2025

@sterliakov your idea worked, thank you! 🤝

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, thanks! Do we need these many command line tests / can we combine them? Command line tests are very slow :-(

@sobolevn
Copy link
Member Author

sobolevn commented Feb 7, 2025

We can combine some of them. Will do!

@sobolevn
Copy link
Member Author

sobolevn commented Feb 7, 2025

I combined several tests together. It made a 2.1s difference, which is great.

Before:

(.venv) ~/Desktop/mypy  issue-18623 ✔                                                     
» pytest  mypy/test/testcmdline.py::PythonCmdlineSuite::cmdline.pyproject.test
=================================== test session starts ===================================
platform darwin -- Python 3.11.9, pytest-8.3.4, pluggy-1.5.0
rootdir: /Users/sobolev/Desktop/mypy
configfile: pyproject.toml
plugins: cov-6.0.0, xdist-3.6.1
12 workers [15 items]     
...............                                                                     [100%]
=================================== 15 passed in 5.20s ====================================

After:

(.venv) ~/Desktop/mypy  issue-18623 ✗                                                  1 ⚠️
» pytest  mypy/test/testcmdline.py::PythonCmdlineSuite::cmdline.pyproject.test
=================================== test session starts ===================================
platform darwin -- Python 3.11.9, pytest-8.3.4, pluggy-1.5.0
rootdir: /Users/sobolev/Desktop/mypy
configfile: pyproject.toml
plugins: cov-6.0.0, xdist-3.6.1
12 workers [11 items]     
...........                                                                         [100%]
=================================== 11 passed in 3.08s ====================================

Some tests are impossible to combine, like ones using files and modules. Basically, this is the best we can do here.

Copy link
Contributor

github-actions bot commented Feb 7, 2025

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@sobolevn
Copy link
Member Author

sobolevn commented Feb 7, 2025

Will wait for a day for extra reviews and going to merge this. Thanks a lot for your ideas and feedback ❤️

Copy link
Collaborator

@tyralla tyralla left a comment

Choose a reason for hiding this comment

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

I played a little with your new try_split function interactively but could not find a case where it failed. So, looks good to me. Thanks!

Copy link
Collaborator

@sterliakov sterliakov left a comment

Choose a reason for hiding this comment

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

LG, thanks!

@sobolevn sobolevn merged commit 5249654 into master Feb 7, 2025
18 checks passed
@sobolevn sobolevn deleted the issue-18623 branch February 7, 2025 23:24
@sobolevn
Copy link
Member Author

sobolevn commented Feb 7, 2025

Thanks everyone! 👏

x612skm pushed a commit to x612skm/mypy-dev that referenced this pull request Feb 24, 2025
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.

Several `pyproject.toml settings are sensitive to trailing comma
4 participants