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 to pre-comit config #334

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Add Pylint to pre-comit config #334

wants to merge 4 commits into from

Conversation

AgentGenie
Copy link
Collaborator

Why are these changes needed?

When reviewing code, I saw many lint errors.
Add pylint to pre-commit to solve this.

Related issue number

Checks

@davorrunje
Copy link
Collaborator

Currently, the repo uses ruff as the default linter in pre-commit and CI. There are some differences between pylint and ruff, as explained here:

At time of writing, Pylint implements ~409 total rules, while Ruff implements over 800, of which at least 209 overlap with the Pylint rule set (see: #970).

Pylint implements many rules that Ruff does not, and vice versa. For example, Pylint does more type inference than Ruff (e.g., Pylint can validate the number of arguments in a function call). As such, Ruff is not a "pure" drop-in replacement for Pylint (and vice versa), as they enforce different sets of rules.

Despite these differences, many users have successfully switched from Pylint to Ruff, especially those using Ruff alongside a type checker, which can cover some of the functionality that Pylint provides.

Like Flake8, Pylint supports plugins (called "checkers"), while Ruff implements all rules natively and does not support custom or third-party rules. Unlike Pylint, Ruff is capable of automatically fixing its own lint violations.

In some cases, Ruff's rules may yield slightly different results than their Pylint counterparts. For example, Ruff's too-many-branches does not count try blocks as their own branches, unlike Pylint's R0912. Ruff's PL rule group also includes a small number of rules from Pylint extensions (like magic-value-comparison), which need to be explicitly activated when using Pylint. By enabling Ruff's PL group, you may see violations for rules that weren't previously enabled through your Pylint configuration.

Pylint parity is being tracked in #970.

My recommendation would be to stick with ruff as the default linter due to its speed. IMO, ff you wish to additionally lint and fix files to comply with pylint as well, that is the best approach.

@AgentGenie AgentGenie marked this pull request as draft January 2, 2025 16:49
@AgentGenie
Copy link
Collaborator Author

@davorrunje Thank you for the detailed comments.
I still want pylint to get the warning and refactor hints ruff doesn't cover. e.g. format, missing doc strings, unused-import etc.
For format, we can use autopep8 to auto fix and comply to pep8.
For other lint issues, my concerns are that we would need some refactoring and may break backward compatibility.

Change this PR to draft for now.

@davorrunje davorrunje self-assigned this Jan 6, 2025
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.

2 participants