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

Log filenames when running pytest-mypy #4502

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Jul 21, 2024

Summary of changes

Reference: realpython/pytest-mypy#93
(please let me know whether you prefer relative or absolute filepaths)

Before: https://github.com/pypa/setuptools/actions/runs/10030930044/job/27720796247?pr=4352#step:9:66

_________________________ setuptools/command/sdist.py __________________________
[gw2] darwin -- Python 3.12.4 /Users/runner/work/setuptools/setuptools/.tox/py/bin/python
46: error: Need type annotation for "negative_opt" (hint: "negative_opt: dict[<type>, <type>] = ...")  [var-annotated]
________________________ setuptools/config/setupcfg.py _________________________
[gw2] darwin -- Python 3.12.4 /Users/runner/work/setuptools/setuptools/.tox/py/bin/python
111: error: Unused "type: ignore" comment  [unused-ignore]
_____________________________ setuptools/errors.py _____________________________
[gw2] darwin -- Python 3.12.4 /Users/runner/work/setuptools/setuptools/.tox/py/bin/python
32: error: Variable "setuptools.errors.OptionError" is not valid as a type  [valid-type]
32: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
32: error: Invalid base class "OptionError"  [misc]
36: error: Variable "setuptools.errors.OptionError" is not valid as a type  [valid-type]
36: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
36: error: Invalid base class "OptionError"  [misc]
40: error: Variable "setuptools.errors.BaseError" is not valid as a type  [valid-type]
40: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
40: error: Invalid base class "BaseError"  [misc]
50: error: Variable "setuptools.errors.BaseError" is not valid as a type  [valid-type]
50: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
50: error: Invalid base class "BaseError"  [misc]
_______________________ setuptools/command/build_ext.py ________________________
[gw1] darwin -- Python 3.12.4 /Users/runner/work/setuptools/setuptools/.tox/py/bin/python
33: error: Unused "type: ignore" comment  [unused-ignore]

After (relative path): https://github.com/pypa/setuptools/actions/runs/10030863175/job/27720645004?pr=4352#step:9:67

________________________ setuptools/config/setupcfg.py _________________________
[gw3] linux -- Python 3.12.4 /home/runner/work/setuptools/setuptools/.tox/py/bin/python
setuptools/config/setupcfg.py:111: error: Unused "type: ignore" comment  [unused-ignore]
_______________________ setuptools/command/build_ext.py ________________________
[gw2] linux -- Python 3.12.4 /home/runner/work/setuptools/setuptools/.tox/py/bin/python
setuptools/command/build_ext.py:33: error: Unused "type: ignore" comment  [unused-ignore]
_____________________________ setuptools/errors.py _____________________________
[gw3] linux -- Python 3.12.4 /home/runner/work/setuptools/setuptools/.tox/py/bin/python
setuptools/errors.py:32: error: Variable "setuptools.errors.OptionError" is not valid as a type  [valid-type]
setuptools/errors.py:32: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
setuptools/errors.py:32: error: Invalid base class "OptionError"  [misc]
setuptools/errors.py:36: error: Variable "setuptools.errors.OptionError" is not valid as a type  [valid-type]
setuptools/errors.py:36: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
setuptools/errors.py:36: error: Invalid base class "OptionError"  [misc]
setuptools/errors.py:40: error: Variable "setuptools.errors.BaseError" is not valid as a type  [valid-type]
setuptools/errors.py:40: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
setuptools/errors.py:40: error: Invalid base class "BaseError"  [misc]
setuptools/errors.py:50: error: Variable "setuptools.errors.BaseError" is not valid as a type  [valid-type]
setuptools/errors.py:50: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
setuptools/errors.py:50: error: Invalid base class "BaseError"  [misc]

After (absolute path): https://github.com/pypa/setuptools/actions/runs/10030898174/job/27720720510?pr=4352#step:9:66

_________________________ setuptools/command/sdist.py __________________________
[gw2] darwin -- Python 3.12.4 /Users/runner/work/setuptools/setuptools/.tox/py/bin/python
/Users/runner/work/setuptools/setuptools/setuptools/command/sdist.py:46: error: Need type annotation for "negative_opt" (hint: "negative_opt: dict[<type>, <type>] = ...")  [var-annotated]
________________________ setuptools/config/setupcfg.py _________________________
[gw2] darwin -- Python 3.12.4 /Users/runner/work/setuptools/setuptools/.tox/py/bin/python
/Users/runner/work/setuptools/setuptools/setuptools/config/setupcfg.py:111: error: Unused "type: ignore" comment  [unused-ignore]
_____________________________ setuptools/errors.py _____________________________
[gw2] darwin -- Python 3.12.4 /Users/runner/work/setuptools/setuptools/.tox/py/bin/python
/Users/runner/work/setuptools/setuptools/setuptools/errors.py:32: error: Variable "setuptools.errors.OptionError" is not valid as a type  [valid-type]
/Users/runner/work/setuptools/setuptools/setuptools/errors.py:32: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
/Users/runner/work/setuptools/setuptools/setuptools/errors.py:32: error: Invalid base class "OptionError"  [misc]
/Users/runner/work/setuptools/setuptools/setuptools/errors.py:36: error: Variable "setuptools.errors.OptionError" is not valid as a type  [valid-type]
/Users/runner/work/setuptools/setuptools/setuptools/errors.py:36: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
/Users/runner/work/setuptools/setuptools/setuptools/errors.py:36: error: Invalid base class "OptionError"  [misc]
/Users/runner/work/setuptools/setuptools/setuptools/errors.py:40: error: Variable "setuptools.errors.BaseError" is not valid as a type  [valid-type]
/Users/runner/work/setuptools/setuptools/setuptools/errors.py:40: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
/Users/runner/work/setuptools/setuptools/setuptools/errors.py:40: error: Invalid base class "BaseError"  [misc]
/Users/runner/work/setuptools/setuptools/setuptools/errors.py:50: error: Variable "setuptools.errors.BaseError" is not valid as a type  [valid-type]
/Users/runner/work/setuptools/setuptools/setuptools/errors.py:50: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
/Users/runner/work/setuptools/setuptools/setuptools/errors.py:50: error: Invalid base class "BaseError"  [misc]

Pull Request Checklist

  • Changes have tests (these are updated tests, see before & after)
  • News fragment added in newsfragments/. (not user facing)
    (See documentation for details)

@Avasam
Copy link
Contributor Author

Avasam commented Jul 30, 2024

@jaraco if this is accepted, should a conftest.py be added to the skeleton ? Also maybe this should even be implemented in jaraco.test ?

@jaraco
Copy link
Member

jaraco commented Aug 1, 2024

I kind-of like the "before" output, given that the filename is given in the header. Adding more details to the output makes it more likely that the most important detail is wrapped or off-screen.

Can you tell me more about the motivation for the change?

If it's a good practice or a best practice, why wouldn't pytest-mypy apply this behavior by default? I tend to try to align with the defaults from upstream and avoid customizing downstream, except where that customization is essentially needed, or documented by the upstream project as "recommended" boilerplate.

if this is accepted, should a conftest.py be added to the skeleton ? Also maybe this should even be implemented in jaraco.test ?

I'd like to avoid adding a conftest.py to the skeleton, because it's an unstructured format and will conflict easily with downstream projects. I'd be much more inclined to implement it as a plugin, such as jaraco.test.

@Avasam
Copy link
Contributor Author

Avasam commented Aug 1, 2024

Can you tell me more about the motivation for the change?

I unironically didn't think to look at the error section header, maybe I was tunnel-visioning hard that day. I think I saw the .EXE path and mentally stopped there.

I do have one functional use that's pretty standard across checker tools that I use: it's convenient to copy setuptools/errors.py:32 so my editor's "go to" command can directly go to the right line.
When the test is run in an editor's shell (Vscode, PyCharm) it also allows clicking the link bringing directly to the right line.

image

It's a small convenience now that I noticed the header (which I feel silly about not noticing earlier). Although one I still have a preference for.

why wouldn't pytest-mypy apply this behavior by default?

Given the filename is in the header, I can only assume they figured it's a good enough minimal default and left any additional information to custom formatter. (ie: let users use the generalized customizable system, rather than adding new configurations)

Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

tl;dr: If this behavior would still be useful to Avasam, let's add it (relative option) with a comment for future retirement. Regardless, let's also file an upstream issue with pytest-mypy (or maybe jaraco.test) to allow for opt-in or opt-out of this behavior (unless pytest-mypy is okay making it the new sole practice).


As I think about it more, I'm thinking this change becomes decreasingly useful as mypy issues are addressed, but it will linger as a divergent behavior indefinitely. How about we add a comment linking back to this PR and propose to consider it for removal in six months (e.g. after 2025-04-01)?

If it's a good practice or a best practice, why wouldn't pytest-mypy apply this behavior by default?

I'd feel much better if this behavior could be applied at the pytest-mypy level. If it's useful for Avasam on Setuptools, the same considerations should apply to other users on other projects. Maybe it should be an option that could be enabled with a flag (or maybe default and disabled by a flag). If we're to accept this change, I'd like to have an issue filed with that project to track that consideration.


Another thing to consider that I hadn't though of before - what do other plugins emit? In my experience, pytest itself shows relative paths and coverage shows relative paths.

collected 9 items                                                                                                                                 

docs/conf.py ....                                                                                                                           [ 44%]
jaraco/compat/py38.py ....                                                                                                                  [ 88%]
. .                                                                                                                                         [100%]

---------- coverage: platform darwin, python 3.12.5-final-0 ----------
Name                    Stmts   Miss  Cover   Missing
-----------------------------------------------------
docs/conf.py                9      0   100%
jaraco/compat/py38.py       8      0   100%
-----------------------------------------------------
TOTAL                      17      0   100%

Why should only pytest-mypy get the absolute path treatment? I don't think it should.

And indeed, when running native mypy, it emits relative paths:

 jaraco.compat main [1] 🐚 mypy .
jaraco/compat/py38.py:36: error: Argument 1 to "startswith" of "str" has incompatible type "int"; expected "str | tuple[str, ...]"  [arg-type]
jaraco/compat/py38.py:37: error: Argument 1 to "len" has incompatible type "int"; expected "Sized"  [arg-type]
Found 2 errors in 1 file (checked 2 source files)

The more I think about it, the less I like this proposal. I personally prefer not to see absolute paths, because it add so much low-value, redundant context and pushes the crucial detail out to the right (to be invisible or wrapped).

I do have one functional use that's pretty standard across checker tools that I use: it's convenient to copy setuptools/errors.py:32 so my editor's "go to" command can directly go to the right line.
When the test is run in an editor's shell (Vscode, PyCharm) it also allows clicking the link bringing directly to the right line.

I don't see how these use-cases benefit from absolute paths. I'd expect editors to understand paths relative to the project root. Is that not the case? Does it really require an absolute path to open a file or make a hyperlink? In your example, you show the editor creating a hyperlink for a relative path. Is that hyperlink broken? If so, that sounds like a bug in the editor, and fixing it just for Setuptools is not sustainable.

Oh! I see now, there are two options for "after" (relative and absolute). I can see the value for relative paths (and not just line numbers). That sounds like a good feature for a plugin or for the plugin to implement as an option.

@Avasam
Copy link
Contributor Author

Avasam commented Aug 29, 2024

I agree absolute paths seem like the worst options of the three. I only included it for completeness sake.

That sounds like a good feature for a plugin or for the plugin to implement as an option.

Given your project setup for skeleton-based projects, which I'm getting more and more accustomed to, I also agree that the logic for this configuration would better live outside setuptools, with a minimal change to setuptools itself (preferably a config)

realpython/pytest-mypy#90 was closed by realpython/pytest-mypy#93 but I don't see any discussion saying that they explicitly wouldn't want to add it as an option.

@Avasam
Copy link
Contributor Author

Avasam commented Sep 12, 2024

Converting to draft since the solution should go outside setuptools (either https://github.com/jaraco/jaraco.test, https://github.com/jaraco/jaraco.develop, or https://github.com/realpython/pytest-mypy)

@Avasam Avasam marked this pull request as draft September 12, 2024 19:16
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.

2 participants