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

chore: add pylint to pre-commit hook #28137

Merged
merged 12 commits into from
Apr 25, 2024
Merged

chore: add pylint to pre-commit hook #28137

merged 12 commits into from
Apr 25, 2024

Conversation

mistercrunch
Copy link
Member

I got bit by the old pylint failing in CI again recently. pylint is too slow for IDEs
so I turn it off since it made vim too sluggish to my taste. Feels about
right as a pre-commit hook, I'd rather get that too take a moment than
waiting for a CI cycle.

Not sure why it wasn't there in the first place, too slow?

We should also consider moving to ruff and streamlining all this, but ruff
doesn't support all the pylint rules [yet].

@github-actions github-actions bot added the github_actions Pull requests that update GitHub Actions code label Apr 19, 2024
@mistercrunch mistercrunch changed the title chore: add pylint to pre-commit hooks chore: add pylint to pre-commit hook Apr 19, 2024
@@ -31,10 +31,6 @@ jobs:
- name: Setup Python
uses: ./.github/actions/setup-backend/
if: steps.check.outputs.python
- name: pylint
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: this other action here https://github.com/apache/superset/blob/master/.github/workflows/pre-commit.yml#L38 will cover for this one.

@john-bodley
Copy link
Member

@mistercrunch per here it seems like this pre-commit hook will only work locally and will be ineffective in a remote CI setting.

@rusackas
Copy link
Member

Seems good to me, but I think @john-bodley might be the right stakeholder approver here.

@mistercrunch
Copy link
Member Author

mistercrunch commented Apr 19, 2024

@john-bodley I think it should be fine as long as I install it in the setup-backend reusable action, it'll install the right/same version as what should be on every dev env too.

I just realized it's only pylinting the superset/ folder, not the tests/ or scripts/. It's really resource-intensive though, brings my M2 macbook to its knees. I think I'll give ruff a shot as it's blazing fast and catches (and autofixes) a bunch of things that other tools don't. They are crushing it on their roadmap and eventually should fully replace pylint/isort/black/pydantic/ .... Coming up in another PR soo

@mistercrunch
Copy link
Member Author

I think turning this on as pre-commit is a clear win if we're going to enforce pylint in CI. It's a bit slow, but pre-commit is a good place to pay the cost of verification (CI is too late, and pylint IDE integrations are sluggish in my experience)

@mistercrunch
Copy link
Member Author

Can I get the @john-bodley seal of approval :)?

I got bit by the old pylint failing in CI again recently. pylint is too slow for IDEs
so I turn it off since it made vim too sluggish to my taste. Feels about
right as a pre-commit hook, I'd rather get that too take a moment than
waiting for a CI cycle.

Not sure why it wasn't there in the first place, too slow?

We should also consider moving to `ruff` and streamlining all this, but ruff
doesn't support all the pylint rules [yet].
@mistercrunch mistercrunch merged commit 7b40b64 into master Apr 25, 2024
27 checks passed
@mistercrunch mistercrunch deleted the pylint-pre-commit branch April 25, 2024 14:58
@john-bodley
Copy link
Member

@mistercrunch regarding your comment,

We should also consider moving to ruff and streamlining all this, but ruff
doesn't support all the pylint rules [yet].

There's a few WIP PRs—by @betodealmeida (I believe) and myself—which have custom Pylint extensions so I'm not sure if/when we'll actually can switch off Pylint as I believe Ruff doesn't support custom lint rules.

@mistercrunch
Copy link
Member Author

mistercrunch commented Apr 25, 2024

@john-bodley I don't think we should deprecate pylint, it seems like it will be a long time before ruff gets to covering most of pylint -> astral-sh/ruff#689 . This means it'll probably take years before it's mostly covered. While pylint is slow, it's a great tool with decades of evolution, and I doubt ruff will even attempt to get to 80%+ there. Unclear how much value is in the long tail (?) But not knowing we should totally keep it going.

One thing that we could consider though is disabling the specific (200+) pylint rules where ruff already covers currently. This could allow pylint to perform better as it's pretty slow currently. This could accelerate commit hooks execution (usually that's not bad as is currently) but significantly speed up pre-commit run --all, which runs in CI and should probably run when doing big rebase/merges.

qleroy pushed a commit to qleroy/superset that referenced this pull request Apr 28, 2024
jzhao62 pushed a commit to jzhao62/superset that referenced this pull request May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code preset-io size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants