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: deprecate pylint in favor of ruff #31262

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Dec 2, 2024

This could be slightly controversial, but submitted as a DRAFT for discussion. After thinking about it, I think we should just move forward with this. Shouldn't be controversial. File under simplify/accelerate builds/devex/tooling.

The case for pylint deprecation:

  • mostly covered by ruff: what matters most in pylint is already re-implemented
  • pylint is SLOW, ruff is a million times faster - pre-commit / pylint is one of our longest, most expensive CI step
  • rules are currently scattered in different places and can conflict at times
  • checked in with the Airflow community, they went all in on ruff, deprecated pylint and are extremely happy with the switch

Very relevant is this link that explains what pylint features are covered or not by ruff:
astral-sh/ruff#970

Essentially we can simplify & speed things up here..

If we agree on the direction, the plan is to follow up and enable all the rules, apply ruff check --fix and ruff check --add-no-qa, which will enable the set of rules commented now, fix the obvious, and add a # noqa to the lines that are not immediately fixable.

This could be slightly controversial, but submitted as a DRAFT for discussion.

The case for pylint deprecation:
- mostly covered by ruff: what matters most in pylint is already re-implemented
- pylint is SLOW, ruff is a million times faster - pre-commit / pylint is one of our longest, most expensive CI step
- rules are currently scattered in different places and can conflict at times

Very relevant is this link that explains what pylint features are covered or not by ruff:
astral-sh/ruff#970

If we agree on the direction, the plan is to follow up and enable all the rules, apply `ruff check --fix` and `ruff check --add-no-qa`

Simplify.
@github-actions github-actions bot added doc Namespace | Anything related to documentation preset-io labels Dec 2, 2024
@mistercrunch mistercrunch marked this pull request as ready for review December 3, 2024 01:38
@michael-s-molina
Copy link
Member

michael-s-molina commented Dec 3, 2024

@mistercrunch Can we reserve this for the 5.0 breaking window? We'll gather proposals for 5.0 next week. I'll send an email and you could include this. I know that this will break our internal release so using a major release is safer.

@michael-s-molina michael-s-molina added the risk:breaking-change Issues or PRs that will introduce breaking changes label Dec 3, 2024
@mistercrunch
Copy link
Member Author

Can we reserve this for the 5.0 breaking window

When is this scheduled? Thinking about it though, switching dev tools shouldn't have any impacts on UX/interfaces. From my understanding semver is mostly interface oriented. I'm favoring moving fast on this while I have momentum on improving devex. Though it may have impact on mergeability of cherries, so may be good to wait for that.

Also noting that when I enabling the pylint rules currently commented in my PR using ruff, it finds hundreds lint issues, meaning turning on ruff should up code quality moving forward (my plan is to enable these rules, run --fix, and --add-noqa).

@mistercrunch
Copy link
Member Author

@sadpandajoe curious on your thoughts about timing, for context the follow up PR would touch the backend with minor linting tweaks, maybe 300 LoC across the backend.

@sadpandajoe
Copy link
Member

@sadpandajoe curious on your thoughts about timing, for context the follow up PR would touch the backend with minor linting tweaks, maybe 300 LoC across the backend.

@mistercrunch I believe the breaking change window is opening soon. I don't think it'll affect Preset but sounds like it might have an affect on AirBnb?

@mistercrunch mistercrunch changed the title chore: deprecarte pylint in favor of ruff chore: deprecate pylint in favor of ruff Dec 3, 2024
@mistercrunch mistercrunch changed the title chore: deprecate pylint in favor of ruff chore: deprecate pylint in favor of ruff Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Namespace | Anything related to documentation preset-io risk:breaking-change Issues or PRs that will introduce breaking changes size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants