-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add Mypy configuration through root "pyproject.toml" file #135
base: master
Are you sure you want to change the base?
Conversation
ea61ad5
to
050fb9b
Compare
@@ -26,6 +26,8 @@ jobs: | |||
run: | | |||
pip install -U pip setuptools wheel | |||
pip install -e . | |||
# Workaround until Mypy regression is fixed. | |||
pip install mypy==1.5.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated to the other changes I made.
Current master
branch is failing with latest version of Mypy for a reason I'm not sure to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please provide a traceback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sobolevn Sure:
$ pytest pytest_mypy_plugins/tests/test-mypy-config.yml -vvv
============================================================================== test session starts ==============================================================================
platform linux -- Python 3.11.5, pytest-7.4.3, pluggy-1.3.0 -- /home/delgan/programming/pytest-mypy-plugins/env/bin/python
cachedir: .pytest_cache
rootdir: /home/delgan/programming/pytest-mypy-plugins
configfile: pyproject.toml
plugins: mypy-plugins-3.0.0
collected 1 item
pytest_mypy_plugins/tests/test-mypy-config.yml::custom_mypy_config_strict_optional_true_set FAILED
=================================================================================== FAILURES ====================================================================================
__________________________________________________________________ custom_mypy_config_strict_optional_true_set __________________________________________________________________
/home/delgan/programming/pytest-mypy-plugins/pytest_mypy_plugins/tests/test-mypy-config.yml:4:
E pytest_mypy_plugins.utils.TypecheckAssertionError: Output is not expected:
E Actual:
E ../../home/delgan/programming/pytest-mypy-plugins/env/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:154: error: Overloaded function signature 2 will never be matched: signature 1's parameter type(s) are the same or broader [misc] (diff)
E Expected:
E (empty)
============================================================================ short test summary info ============================================================================
FAILED pytest_mypy_plugins/tests/test-mypy-config.yml::custom_mypy_config_strict_optional_true_set -
=============================================================================== 1 failed in 3.14s ===============================================================================
This can be reproduced by executing mypy --no-silence-site-packages --no-strict-optional
on the following file:
from typing import Optional
a: Optional[int] = None
a + 1 # should not raise an error
Mypy v1.6.0+ flags this snippet as invalid and exits with non-zero:
env/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:154: error: Overloaded function signature 2 will never be matched: signature 1's parameter type(s) are the same or broader [misc]
Found 1 error in 1 file (checked 1 source file)
This doesn't happen when --no-silence-site-packages
is omitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--no-strict-optional
is not tested much in mypy (and for a good reason - it should not be used).
How does it affect us here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used to verify that inline mypy_config
works as expected: https://github.com/typeddjango/pytest-mypy-plugins/blob/master/pytest_mypy_plugins/tests/test-mypy-config.yml
We can probably test it with another option, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stumbled upon this pr because tests failed with mypy-1.8.0 (https://bugs.gentoo.org/921901). (Where) has the regression been reported upstream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a full review for now :)
@@ -195,6 +195,16 @@ mypy-tests: | |||
|
|||
``` | |||
|
|||
## Configuration | |||
|
|||
For convenience, it is also possible to define a default `mypy` configuration in the root `pyproject.toml` file of your project: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, also describe what priority this setting has over CLI flags / etc.
@@ -54,7 +61,7 @@ def factory(filename: Optional[str], expected: str) -> None: | |||
def test_join_existing_config( | |||
execution_path: Path, assert_file_contents: _AssertFileContents, additional_config: str | |||
) -> None: | |||
filepath = join_toml_configs(_PYPROJECT1, additional_config, execution_path) | |||
filepath = join_toml_configs(_MYPY_PLUGINS_CONFIG2, _PYPROJECT1, additional_config, execution_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's please keep existing tests as-is :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the new implementation, I had no other choice but to add a new parameter to join_toml_configs()
, and consequently I had to update the tests. What alternative approach would you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine. 👍
Also, to test precedence, I added a parameter to the pyproject1.toml
:
show_error_context = true |
Consequently, the output of some existing tests changed as well:
pytest-mypy-plugins/pytest_mypy_plugins/tests/test_configs/test_join_toml_configs.py
Line 99 in 050fb9b
show_error_codes = false |
Do you want this to be reverted as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please. I am asking this, because I want to make sure it won't affect existing use-cases for existing users.
@@ -0,0 +1 @@ | |||
# This file has no `[tool.pytest-mypy-plugins.mypy-config]` existing config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just update the comment in https://github.com/typeddjango/pytest-mypy-plugins/blob/master/pytest_mypy_plugins/tests/test_configs/pyproject2.toml
No need to create two empty files.
Thanks a lot! |
3ec2297
to
e7b7a9d
Compare
@sobolevn I updated the PR according to your comments. 👍 Two remarks waiting for your opinion:
|
It is not uncommon to require a Mypy configuration that differs from the project's main configuration and is specific to tests, such as enabling the 'force_uppercase_builtins' option. Currently, the argument '--mypy-pyproject-toml-file' can be used via the command line, but this approach has two drawbacks: - It requires an additional file in the codebase, whereas it is more pleasant to group all configurations in the root 'pyproject.toml' file. - It confines the invocation of 'pytest' to a fixed location, as the path is resolved relative to the current working directory. However, there are situations where it is useful to call 'pytest' from a different directory. The solution implemented here allows for configuring the Mypy parameters used by 'pytest-mypy-plugins' directly within the project's 'pyproject.toml' file, addressing both of the aforementioned points.
e7b7a9d
to
0f10c94
Compare
Hi!
This is a proposal to fix #133.
This offers a way to use Mypy configuration specific to tests without needing the
--mypy-pyproject-toml-file
command line argument. Instead, users can define a[tool.pytest-mypy-plugins.mypy-config]
table in their repository'spyproject.toml
file.This approach addresses two issues:
pytest
can be invoked from any location (this is not possible when--mypy-pyproject-toml-file
is specified via theaddopts
configuration, as paths are resolved relative to the current working directory).The order of precedence for Mypy configuration is as follows, with each subsequent option taking precedence over the previous ones if it exists:
[tool.pytest-mypy-plugins.mypy-config]
in the rootpyproject.toml
as default.--mypy-pyproject-toml-file
or--mypy-ini-file
if provided.config_mypy
section in a test if present.I hope this process isn't too convoluted. Please let me know if you prefer a different approach. I made slight modifications to the
join_toml_configs()
function to implement the rules mentioned above. Additionally, I can add more unit tests if necessary.