archinstall icon indicating copy to clipboard operation
archinstall copied to clipboard

Add Pylint support and enable Pylint CI checks

Open correctmost opened this issue 1 year ago • 14 comments

PR Description:

Pylint can help catch issues like #2625:

************* Module archinstall.default_profiles.server
archinstall/default_profiles/server.py:34:4: E1101: Instance of 'ServerProfile' has no 'set_current_selection' member (no-member)

I didn't add the Pylint checks to the pre-commit config because they might be a bit too slow to run on each commit.

Tests and Checks

  • [ ] I have tested the code!

correctmost avatar Aug 29 '24 12:08 correctmost

Maybe I'm missing something here but why is there a need for ruff and pylint? Isn't ruff covering everything?

svartkanin avatar Aug 29 '24 13:08 svartkanin

Pylint catches bugs that ruff misses because ruff doesn't analyze code across multiple files (https://github.com/astral-sh/ruff/issues/7447). The ServerProfile bug is one example where Pylint analyzes the base class and detects the missing member, which ruff currently does not detect.

Pylint also catches bugs that mypy misses because Pylint doesn't automatically trust type annotations.

https://docs.astral.sh/ruff/faq/#how-does-ruffs-linter-compare-to-pylint has some additional details.

correctmost avatar Aug 29 '24 13:08 correctmost

Before we merge this, I have to create a python-pylint-pydantic package as we need to be able to source this during PKGBUILD:

makedepends=(
  'python-setuptools'
  'python-sphinx'
  'python-build'
  'python-installer'
  'python-wheel'
+ 'python-pylint'
+ 'python-pylint-pydantic'
)

However that package does not exist currently in any repo, not even in AUR.

It's not a major showstopper, but I just need to sit down a few minutes and make that happen.

Torxed avatar Aug 29 '24 14:08 Torxed

@Torxed why is it needed for a build? Those packages are part of the dev packages in pyproject.toml so they shouldn't be dependencies for a build?

svartkanin avatar Aug 29 '24 14:08 svartkanin

Because we should run check() in PKGBUILD. I haven't been a model packaging citizen, but now that we're doing good stuff this should ideally be there :)

Torxed avatar Aug 29 '24 14:08 Torxed

Hmm, is there a way to pin versions of dev dependencies in PKGBUILD?

One scenario that comes to mind:

  1. CI checks run and pass with Pylint 3.2.x
  2. The Arch package gets updated to 3.3.x, which detects an error that previously went unnoticed by earlier versions
  3. The package fails to build as a result of the improved checks

I wouldn't want the linters to cause packaging issues down the line.

correctmost avatar Aug 29 '24 15:08 correctmost

Hmm I'm still somewhat worried that mixing the build with linting may cause issues down the line. If these checks run on PRs and only PRs that validate successfully get merged then the master branch should be considered ckean and ready for a build. I remember @Torxed in the past rare PRs got merged with failing checks coz fixing a bug was higher priority then meeting linting checks. If such a check is now failing and the PR gets merged it'll block a hit fix release completely.

svartkanin avatar Aug 29 '24 16:08 svartkanin

Hmm I'm still somewhat worried that mixing the build with linting may cause issues down the line. If these checks run on PRs and only PRs that validate successfully get merged then the master branch should be considered ckean and ready for a build. I remember @Torxed in the past rare PRs got merged with failing checks coz fixing a bug was higher priority then meeting linting checks. If such a check is now failing and the PR gets merged it'll block a hit fix release completely.

There's also the odd "arch packaging patches" where any trusted user can go in and submit -<arch patch> versions.

Which is what you see sometimes where, a package didn't change version upstream like v2.8.6 might be packaged with v2.8.6-2 only in arch.

It's a low probability of issues, but best practice in packaging is to have the check() hook during packaging that should just run a final test/linting.

And it's no problem for me to make a PKGBUILD for pylint-pydantic as long as it's maintained upstream :)

I just need to find my way to my laptop 😅

Torxed avatar Aug 29 '24 16:08 Torxed

Maybe we should pause the pylint addition for a bit and see how well ruff in combination with the others work, get mypy fully compliant and see if we need pylint in addition?

svartkanin avatar Aug 29 '24 18:08 svartkanin

My preference would be to enable Pylint checks without the PKGBUILD changes for now. The checks will help validate the fixes for the remaining mypy and ruff violations and prevent new issues from being introduced.

I think a lot of the remaining violations are going to require code changes as opposed to just annotation changes, so having the additional linting checks will help detect regressions.

I can start fixing more issues next week, but I think it will still take a while to be fully mypy compliant.

correctmost avatar Aug 29 '24 19:08 correctmost

It's true, we could skip the check() in PKGBUILD's for now. But knowing myself it will probably be forgotten until someone pokes me or when it bites me in the butt.

But the tests that pylint adds sounds nice tbh.

Torxed avatar Aug 29 '24 19:08 Torxed

@correctmost I'm soon (TM) about to raise an integration PR for the new menu system which is going to shuffle things around, so just to prevent any throwaway work being done just ignore any menu related linting

svartkanin avatar Aug 30 '24 05:08 svartkanin

https://github.com/archlinux/archinstall/pull/2663

svartkanin avatar Aug 31 '24 10:08 svartkanin

I've successfully built the dependencies now. I just need to sign and publish the packages :)

Edit: Been on vacation for a week, but picking this up again.

Torxed avatar Sep 06 '24 06:09 Torxed

I've now managed to package python-pylint-pydantic. I appreciate the work done here, and I can only apologize that it took a while to get around to packaging the new package and merge this.

Torxed avatar Nov 04 '24 13:11 Torxed