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

Add Pylint support and enable Pylint CI checks #2658

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

correctmost
Copy link
Contributor

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!

@svartkanin
Copy link
Collaborator

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

@correctmost
Copy link
Contributor Author

Pylint catches bugs that ruff misses because ruff doesn't analyze code across multiple files (astral-sh/ruff#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.

@Torxed
Copy link
Member

Torxed commented Aug 29, 2024

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.

@svartkanin
Copy link
Collaborator

@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?

@Torxed
Copy link
Member

Torxed commented Aug 29, 2024

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 :)

@correctmost
Copy link
Contributor Author

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.

@svartkanin
Copy link
Collaborator

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.

@Torxed
Copy link
Member

Torxed commented Aug 29, 2024

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 😅

@svartkanin
Copy link
Collaborator

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?

@correctmost
Copy link
Contributor Author

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.

@Torxed
Copy link
Member

Torxed commented Aug 29, 2024

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.

@svartkanin
Copy link
Collaborator

@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
Copy link
Collaborator

#2663

@correctmost correctmost force-pushed the cm/pylint-support branch 3 times, most recently from 575a4b8 to 653f99e Compare September 6, 2024 00:57
@Torxed
Copy link
Member

Torxed commented Sep 6, 2024

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
Copy link
Member

Torxed commented Nov 4, 2024

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 Torxed merged commit aecf3ea into archlinux:master Nov 4, 2024
8 checks passed
@correctmost correctmost deleted the cm/pylint-support branch November 4, 2024 13:57
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.

3 participants