conan-center-index icon indicating copy to clipboard operation
conan-center-index copied to clipboard

Maintenance: remove outdated Conan 2 linter

Open jcar87 opened this issue 9 months ago • 7 comments

No longer relevant:

  • Conan 2 is mandatory in all PRs, so invalid syntax will be immediately shown.

Historically, this was useful when there was no Conan 2.0 pipeline running.

jcar87 avatar Apr 25 '24 12:04 jcar87

Once again, pylint is very useful to catch bugs. please consider https://github.com/conan-io/conan-center-index/pull/23758 which should drop conan1 support for the linter

ericLemanissier avatar Apr 25 '24 12:04 ericLemanissier

Once again, pylint is very useful to catch bugs. please consider #23758 which should drop conan1 support for the linter

Could you provide examples of bugs that have been recently raised by the linter where this has been useful? I'm assuming these fall under the category of "all methods in the recipe actually work, but it may not be working as intended?

jcar87 avatar Apr 25 '24 13:04 jcar87

any typo in variable/attribute/function name in a non-taken branch can only caught by a static analyzer.

not very recent, but still: https://github.com/conan-io/conan-center-index/pull/7749#issuecomment-982468172

https://github.com/conan-io/conan-center-index/pull/11238 https://github.com/conan-io/conan-center-index/pull/11239 https://github.com/conan-io/conan-center-index/pull/11213 https://github.com/conan-io/conan-center-index/pull/22158/commits/983176e1c4d6a40bd130a32e851505fb2cc3af87

ericLemanissier avatar Apr 25 '24 13:04 ericLemanissier

I meant recent examples where the linter is actively reporting actual recipe bugs in GitHub PRs. Happy to consider the removal on that basis.

We are currently working on a revamped pipeline, which will centralise all reporting (linting and builds, and all checks) - so functionality will be assumed and reported by our CI.

jcar87 avatar Apr 25 '24 13:04 jcar87

Maybe you did not see the edits to my message ?

ericLemanissier avatar Apr 25 '24 13:04 ericLemanissier

Maybe you did not see the edits to my message ?

PRs from 2 years ago that look like manual runs of the linter on code that was checked in before that?

I'm not arguing the linter isn't useful - but I want to measure how often and how recently and exactly what sort of errors it is catching. Otherwise, if it's covering low probability stuff, we can live with it disabled until our new implementation.

jcar87 avatar Apr 25 '24 14:04 jcar87

Can you please update the description of the PR so that the community understands why this change is needed ? You are not removing outdated conan 2 linters, you are removing ALL PYTHON LINTERS. You say No longer relevant: which is wrong. I agree that the linters that exist only to help transition to conan 2 can be dropped, and I proposed the fix in https://github.com/conan-io/conan-center-index/pull/23758. you say Historically, this was useful when there was no Conan 2.0 pipeline running. which is also wrong, pytlint was considered and implemented before conan 2 migration tools and Dropping all other pylints checks is a very serious reduction on the quality of conan-center recipe. I obviously cannot go through all the runs of the checkers, with more than 50 runs per day. One recent actual bug catch was in https://github.com/conan-io/conan-center-index/commit/983176e1c4d6a40bd130a32e851505fb2cc3af87#diff-89f04a00c7ace6261a53[…]25526ec09f965d57fa2bf7R97 which from https://github.com/conan-io/conan-center-index/pull/23437 (but it is not visible in the PR because the author force pushed) Let me repost examples of bugs which actually where merged in CCI, because pylint was not yet implemented:

  • double configure method : https://github.com/ericLemanissier/conan-center-index/commit/a56405fdf49ef4139f954840150a413175fff316
  • usage of square brackets instead of parentheses : https://github.com/ericLemanissier/conan-center-index/commit/2d02614d1faf1ac6af47a862d3da926340c88a71
  • double validate method : https://github.com/jgsogo/conan-center-index/commit/f0964c08cb920880c794c2593dfa28ac0a8f6a1e
  • double validate method : https://github.com/jgsogo/conan-center-index/commit/6204d71a05121c56f9d1ef2a8548b034af88921d
  • double configure method: https://github.com/conan-io/conan-center-index/pull/936#discussion_r383519485 -this one was not merged, but only caught after 20 minutes of review)
  • missing import https://github.com/conan-io/conan-center-index/pull/7696/files#diff-75b9f888a7159f587df53d73476654254af05ca90d777c2525335ec2d2c520d2R2

In addition to catching bug which are not detected other wise, running pylint has a great advantage: It provides very quick feedback (in the 20 seconds ballpark) on syntax errors, in a very readable way on github (directly in the source code): image. It also works when C3I is in maintenance or out of order.

Also, it does not cost you anything, it does not run on your CI servers, it does not add any traffic on conan-center, and the maintenance is very low (see the number of commits in https://github.com/conan-io/conan-center-index/commits/master/linter and https://github.com/conan-io/conan-center-index/commits/master/.github/workflows/linter-conan-v2.yml)

Removing pylint is a net loss in quality, and I see no valid reason to take this move.

ericLemanissier avatar Apr 25 '24 15:04 ericLemanissier