archinstall
archinstall copied to clipboard
Add Pylint support and enable Pylint CI checks
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!
Maybe I'm missing something here but why is there a need for ruff and pylint? Isn't ruff covering everything?
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.
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 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?
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 :)
Hmm, is there a way to pin versions of dev dependencies in PKGBUILD?
One scenario that comes to mind:
- CI checks run and pass with Pylint 3.2.x
- The Arch package gets updated to 3.3.x, which detects an error that previously went unnoticed by earlier versions
- 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.
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.
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 😅
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?
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.
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.
@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
https://github.com/archlinux/archinstall/pull/2663
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.
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.