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

Make more linters happy #277

Merged
merged 3 commits into from
Mar 4, 2024
Merged

Make more linters happy #277

merged 3 commits into from
Mar 4, 2024

Conversation

andlaus
Copy link
Collaborator

@andlaus andlaus commented Mar 4, 2024

This fixes some complaints brought up by flake8, pytype and pyright. Note that I don't intend to run these linters as part of the CI, because it would make contributing even harder than it already is and there are some issues with flake8 and pytype:

  • flake8 likes to play "style police" and thus requires a ton of parameters (--ignore E124,E125,E126,E131,E501,F541,W503,W504), which together with the fact that it cannot (easily) be configured via pyproject.toml makes it quite cumbersome.
  • pytype is glacially slow and produces a few false positives which I could not fix
  • pyright fights with mypy about the import of InquirerPy.prompt in the browse tool, so it draws the shorter stick...

Andreas Lauser <[email protected]>, on behalf of MBition GmbH.
Provider Information

this supposes that it is run using

```
flake8 --ignore E124,E125,E126,E131,E501,F541,W503,W504 odxtools tests
```

Note that flake8 does not support configuration using
`pyproject.toml`, so setting these parameters by default would require
a new `.flake8` file in the repository. That said, IMO providing a
pre-packaged config for `flake8` is mildly undesireable anyway because
I think `flake8` should continue not be run as part of the CI system
(mainly because it plays style police and it clashes with our yapf
output but also because it would further raise the burden for getting
pull requests merged) and thus the file would almost certainly bitrot.

Signed-off-by: Andreas Lauser <[email protected]>
Signed-off-by: Christian Hackenbeck <[email protected]>
@andlaus andlaus requested a review from kayoub5 March 4, 2024 13:21
pytype in particular seems to be missing a few features and is
glacially slow...

Signed-off-by: Andreas Lauser <[email protected]>
Signed-off-by: Christian Hackenbeck <[email protected]>
@kayoub5
Copy link
Collaborator

kayoub5 commented Mar 4, 2024

@andlaus I would recommend dropping flake8 in favor of https://github.com/astral-sh/ruff

@@ -4,7 +4,7 @@
import sys
from typing import List, Optional, Union, cast

import InquirerPy.prompt as PI_prompt
import InquirerPy.prompt as IP_prompt
Copy link
Collaborator

Choose a reason for hiding this comment

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

What? are you sure?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the history of this is that in its current state pyright, complains about this:

$ pyright
/home/and/src/odxtools/odxtools/cli/browse.py
  /home/and/src/odxtools/odxtools/cli/browse.py:7:8 - error: Import "InquirerPy.prompt" could not be resolved (reportMissingImports)
  /home/and/src/odxtools/odxtools/cli/browse.py:100:14 - error: Module is not callable (reportCallIssue)
[...]

while if this issue is fixed by replacing the line with from InquirerPy import prompt as IP_prompt # type: ignore[attr-defined], mypy complains about the # type: ignore comment being unnecessary (at least on the GH actions runner, locally it is fine on my machine). the PI to IP rename is a residual of that, because I thought that if I have to change that line anyway, I could use a name that makes sense...

@andlaus
Copy link
Collaborator Author

andlaus commented Mar 4, 2024

thanks for the hint, I'll have a look. Be aware, though that I only intend to run these linters every blue moon, and only manually..

@andlaus
Copy link
Collaborator Author

andlaus commented Mar 4, 2024

BTW: we already use ruff as part of the CI system ;)

as usual, thanks to [at]kayoub5 for the finds.

Signed-off-by: Andreas Lauser <[email protected]>
Signed-off-by: Christian Hackenbeck <[email protected]>
@andlaus andlaus merged commit 0614e2b into mercedes-benz:main Mar 4, 2024
7 checks passed
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