Pytest: Static analysis: pylint integration

[anonyknight]

• [ ] Static analysis is a good way to gate-keep software quality. From .pre-commit-config.yaml, I can see pytest enabled flake8 for pep8, pep257 checker. Is there any reason to skip pylint?

• [ ] pytest and operating system versions: pytest-5.4.1.dev127+gf479cbce1

• [ ] minimal example if possible
  e.g. I can see valid static risks in src/_pytest/junitxml.py
  {
  "resource": "/home/will/code/pytest/src/_pytest/junitxml.py",
  "owner": "python",
  "code": "consider-iterating-dictionary",
  "severity": 8,
  "message": "Consider iterating the dictionary directly instead of calling .keys()",
  "source": "prospector - pylint",
  "startLineNumber": 150,
  "startColumn": 20,
  "endLineNumber": 150,
  "endColumn": 20
  }
  {
  "resource": "/home/will/code/pytest/src/_pytest/junitxml.py",
  "owner": "python",
  "code": "import-outside-toplevel",
  "severity": 8,
  "message": "Import outside toplevel (_pytest.warning_types)",
  "source": "prospector - pylint",
  "startLineNumber": 265,
  "startColumn": 5,
  "endLineNumber": 265,
  "endColumn": 5
  }

[bluetech]

Is there any reason to skip pylint

Adding a second linter adds some maintenance overhead - in setup (CI, tox and by each developer) and in development (add ignores and such) and overhead (time it takes to run).

It might be worth it if there is sufficient added value. Would you be able to demonstrate the warnings it triggers? Regarding the two you pasted:

1. "Consider iterating the dictionary directly instead of calling .keys()" - :+1:
2. "Import outside toplevel" - these are almost always intentional, either to avoid circular dependencies, or to intentionally lazy-load.

I think the best way to move forward with this is to open a PR with lint fixes suggested by pylint, to demonstrate its value.

[symonk]

Can certainly have a look, I think there is a fine line where having too many starts to become somewhat painful as a maintenance overhead, e.g something that is considered 'OK' on linter A, fails on B and vice versa, so we have to be careful not to be too over zealous (imo)

[RonnyPfannschmidt]

a key reason why pylint was never added is that in the past it was insanely chatty with insane amounts of false positives/nitpicks - given that in general black is used these days + tools to do such cleanups its hard or me to see value in adding pylint

[gnikonorov]

@anonyknight does @RonnyPfannschmidt's answer above answer your question?

[symonk]

just to revisit; -1 from me on this one. I think what we have is sufficient for the project