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

Migrate pylint to ruff #57182

Merged
merged 2 commits into from
Apr 8, 2024
Merged

Migrate pylint to ruff #57182

merged 2 commits into from
Apr 8, 2024

Conversation

tqa236
Copy link
Contributor

@tqa236 tqa236 commented Jan 31, 2024

Hi @mroeschke, while looking into the pylint rule, I noticed that most of them are currently gated behind the preview flag. Do you think we should enable the preview flag or wait some rules to be promoted to stable in ruff==0.2.0 (should be released in the next few weeks)

Other than that, this PR is ready for review.

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

@mroeschke
Copy link
Member

Is it possible to opt in to preview mode with just pylint?

@tqa236
Copy link
Contributor Author

tqa236 commented Jan 31, 2024

@mroeschke can you take a look at the latest commit and the 177 errors that it detects? I think we can achieve that with pre-commit, although we need to explicitly pass all necessary.

This new hook replaces the one below, which is disabled by default

-   repo: https://github.com/pylint-dev/pylint
    rev: v3.0.1
    hooks:
    -   id: pylint
        stages: [manual]
        args: [--load-plugins=pylint.extensions.redefined_loop_name]

Given that I can't fix all the errors within 1 PR, let me know how should I proceed

@mroeschke
Copy link
Member

mroeschke commented Jan 31, 2024

My main curiosity is how many of the 177 errors are ruff-pylint checks that are not enabled in the current pylint checks?

[tool.pylint.messages_control]

Ideally this PR should just bump ruff and enable ruff-pylint, and all modifications are updates from other rules.

Then in a follow up PR, the old pylint can be removed (and optional ruff-pylint rules can be enabled too)

@mroeschke mroeschke added the Code Style Code style, linting, code_checks label Feb 1, 2024
@tqa236 tqa236 changed the title Bump ruff to version 0.1.15 Bump ruff to version 0.2.0 Feb 1, 2024
@tqa236 tqa236 marked this pull request as draft February 5, 2024 20:15
@tqa236 tqa236 force-pushed the bump-ruff branch 2 times, most recently from 18c4208 to 893697e Compare February 6, 2024 00:49
@tqa236 tqa236 changed the title Bump ruff to version 0.2.0 Bump ruff to version 0.2.1 Feb 6, 2024
@tqa236 tqa236 marked this pull request as ready for review February 6, 2024 00:51
@tqa236
Copy link
Contributor Author

tqa236 commented Feb 6, 2024

hi @mroeschke, please take a look at the last commit, where I manage to make an automatic translation of pylint config to ruff config with the help of pylint-to-ruff.

Given the list of pylint rules that has not been implemented in ruff (the one that's commented out), let me know if it's ok to drop pylint in favor of ruff

pyproject.toml Outdated
# Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
# "E721",
# Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
"RUF021",
Copy link
Member

Choose a reason for hiding this comment

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

To confirm, are these rules pylint rules that are currently ignore in pandas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @mroeschke, it's not. They are some left over rules while I'm trying to figure out how to run preview mode with pylint only. I pushed a new commit to make the intent clearer.

Now in pyproject.toml, you can find 2 sections, "Output of pylint-to-ruff", where the pylint config is automatically translated to ruff and "Additional pylint rules" are the extra rules we need to ignore to make the check pass. I think most of these rules are new rules that taken inspiration from pylint's existing rules.

Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Thanks looks pretty good. I don't think the commented-out rules are blockers for removing pylint in a follow up. Could you merge in main once more before we merge?

@mroeschke mroeschke added this to the 3.0 milestone Feb 7, 2024
@tqa236
Copy link
Contributor Author

tqa236 commented Feb 7, 2024

@mroeschke thanks for your review. I rebased on main

@@ -30,11 +30,11 @@ repos:
files: ^pandas
exclude: ^pandas/tests
args: [--select, "ANN001,ANN2", --fix-only, --exit-non-zero-on-fix]
- id: ruff
args: [--exit-non-zero-on-fix, --preview, "--ignore=E,F841,RUF"]
Copy link
Member

Choose a reason for hiding this comment

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

Can this just select pylint instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will override both the select and ignore section in pyproject.toml and select all pylint rules instead. I can't figure out a way to select only pylint rules while still respect the ignore section.

How about adding a comment saying that only pylint rules need to be enabled in this hook and the rest can be ignored? We'll just need to update the ignore list when ruff is updated and something fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just discovered this PR where ruff will allow us to override any section of the config with the command line, thus achieving our goal here. Given that the PR is not merged yet and the current hooks work for the short term, maybe I can add a TODO to clean it up later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mroeschke I tried the new config override feature in ruff with [--exit-non-zero-on-fix, --preview, --config, "lint.select=['PL']"] but it still doesn't work for our case because both the select and ignore section in pyproject.toml will be resolved first, then they will be overriden together by --config, "lint.select=['PL']" in the command line, which result in a similar outcome to passing --select=PL directly to the command line. So I keep the old method to keep the pipeline passed.

Let me know what I should do next to get this PR merged.

@@ -30,11 +30,11 @@ repos:
files: ^pandas
exclude: ^pandas/tests
args: [--select, "ANN001,ANN2", --fix-only, --exit-non-zero-on-fix]
- id: ruff
Copy link
Member

Choose a reason for hiding this comment

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

If we need a new section for pylint for --preview, could you rename this ruff-pylint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The id is defined in ruff-pre-commit, not a name we can change, changing this will lead to the error below.

[ERROR] `ruff-pylint` is not present in repository https://github.com/astral-sh/ruff-pre-commit.  Typo? Perhaps it is introduced in a newer version?  Often `pre-commit autoupdate` fixes this.

@tqa236 tqa236 force-pushed the bump-ruff branch 3 times, most recently from c1ce2c3 to b3508fd Compare February 18, 2024 00:08
@tqa236 tqa236 changed the title Bump ruff to version 0.2.1 Bump ruff to version 0.2.2 Feb 18, 2024
@tqa236
Copy link
Contributor Author

tqa236 commented Mar 7, 2024

hi @mroeschke, I wonder if we should split the version bump of ruff with the switch from pylint to ruff so that we can use the latest ruff version first.

I can also take a look at the rather messy bump from 0.2.0 to 0.3.0 if it's good for you

@mroeschke
Copy link
Member

Yes I think that's a good idea to bump ruff to 0.3.0 first before doing a pylint migration

@tqa236 tqa236 changed the title Bump ruff to version 0.2.2 Migrate pylint to ruff Mar 12, 2024
@tqa236 tqa236 marked this pull request as draft March 12, 2024 06:37
@tqa236 tqa236 force-pushed the bump-ruff branch 2 times, most recently from 858c803 to 012c68a Compare March 12, 2024 18:53
@mroeschke
Copy link
Member

Coming back around to this; to simplify the transition I think we can just enable the pylint rule in ruff (and ignore the non-fix rules that don't pass currently) and remove the pylint config in pre-commit-config. I think it's OK to not exactly have a 1:1 transition between the old and new pylint configuration

@mroeschke mroeschke marked this pull request as ready for review April 8, 2024 21:24
@mroeschke mroeschke merged commit d460abd into pandas-dev:main Apr 8, 2024
46 checks passed
@mroeschke
Copy link
Member

Thanks @tqa236

@tqa236 tqa236 deleted the bump-ruff branch April 9, 2024 07:07
@tqa236
Copy link
Contributor Author

tqa236 commented Apr 9, 2024

thank you for finalizing this PR, I was on vacation and didn't have time to check your last message.

pmhatre1 pushed a commit to pmhatre1/pandas-pmhatre1 that referenced this pull request May 7, 2024
Configure ruff pylint

Co-authored-by: Matthew Roeschke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants