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

Split ruff backend into separate backends for formatting and linting #20545

Merged
merged 28 commits into from
Feb 16, 2024

Conversation

krishnan-chandra
Copy link
Contributor

@krishnan-chandra krishnan-chandra commented Feb 14, 2024

Closes #20525.

This PR splits Ruff formatting out into a separate backend from Ruff linting/fixing, and registers the new backend as an experimental one. For backwards compatibility, we retain the existing lint/fix backend at pants.backend.experimental.python.lint.ruff and move the formatting backend out to pants.backend.experimental.python.lint.ruff_fmt.

I tried to keep the changes as minimal as possible and not touch too many files / move too many things around.

This will be a breaking change for anyone who has adopted the Ruff formatter in 2.20 pre-release, but that should be okay as the feature hasn't been released yet.

The naming of the new backend is a bit confusing, but since all other formatter backends are published under the lint package I tried to keep the convention consistent.

@krishnan-chandra krishnan-chandra added category:internal CI, fixes for not-yet-released features, etc. backend: Python Python backend-related issues labels Feb 14, 2024
@krishnan-chandra krishnan-chandra changed the title Split ruff backend Split ruff backend into two Feb 14, 2024
@krishnan-chandra krishnan-chandra changed the title Split ruff backend into two Split ruff backend into separate backends for formatting and linting Feb 14, 2024
@krishnan-chandra krishnan-chandra marked this pull request as ready for review February 14, 2024 13:41
@krishnan-chandra
Copy link
Contributor Author

Testing this on my own work repo - this does seem to work as intended.

Without the new backend enabled:

PANTS_SOURCE=~/Projects/pants pants fix lint ::

10:47:35.41 [INFO] Completed: Format with shfmt - shfmt made no changes.
10:47:37.16 [INFO] Completed: Fix with Autoflake - autoflake made no changes.
10:47:37.17 [INFO] Completed: Fix with `ruff check --fix` - ruff check --fix made no changes.

✓ autoflake made no changes.
✓ ruff check --fix made no changes.
✓ shfmt made no changes.

==> Linting <chart>
[INFO] Chart.yaml: icon is recommended
1 chart(s) linted, 0 chart(s) failed

10:47:37.36 [INFO] Completed: Lint with `ruff check` - ruff check succeeded.
10:47:37.46 [INFO] Completed: Lint with Shellcheck - shellcheck succeeded.
10:47:37.47 [INFO] Completed: Lint Helm charts - helm succeeded.

✓ autoflake succeeded.
✓ helm succeeded.
✓ ruff check succeeded.
✓ shellcheck succeeded.
✓ shfmt succeeded.

With the new backend enabled:

PANTS_SOURCE=~/Projects/pants pants fix lint ::

10:49:13.64 [INFO] Completed: Format with shfmt - shfmt made no changes.
10:49:15.52 [INFO] Completed: Fix with Autoflake - autoflake made no changes.
10:49:15.53 [INFO] Completed: Fix with `ruff check --fix` - ruff check --fix made no changes.
10:49:15.87 [WARN] Completed: Format with `ruff format` - ruff format made changes.

✓ autoflake made no changes.
✓ ruff check --fix made no changes.
+ ruff format made changes.
✓ shfmt made no changes.

==> Linting <chart>
[INFO] Chart.yaml: icon is recommended
1 chart(s) linted, 0 chart(s) failed

10:49:16.61 [INFO] Completed: Format with `ruff format` - ruff format made no changes.
10:49:16.63 [INFO] Completed: Lint with `ruff check` - ruff check succeeded.
10:49:16.71 [INFO] Completed: Fix with Autoflake - autoflake made no changes.

✓ autoflake succeeded.
✓ helm succeeded.
✓ ruff check succeeded.
✓ ruff format succeeded.
✓ shellcheck succeeded.
✓ shfmt succeeded.

Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

sweet. minor suggestions only.

@huonw
Copy link
Contributor

huonw commented Feb 14, 2024

Nice! Thanks for this. I like the arrangement, although, as an alternative option, what do you think about having the backends mirror the ruff CLI?

  • pants.backend.experimental.python.lint.ruff.check (or maybe ruff_check)
  • pants.backend.experimental.python.lint.ruff.format

Then, to help users migrate pants.backend.experimental.python.lint.ruff can just be a "reexport" of the rules from pants.backend.experimental.python.lint.ruff.check, but with a deprecation.

I'm trying to think to "how would we do it if were just adding a ruff backend now", and I'm not sure we'd "prioritise" ruff check over ruff format? But maybe we would, I don't know! Thoughts?

@cognifloyd
Copy link
Member

Nice! Thanks for this. I like the arrangement, although, as an alternative option, what do you think about having the backends mirror the ruff CLI?

  • pants.backend.experimental.python.lint.ruff.check (or maybe ruff_check)
  • pants.backend.experimental.python.lint.ruff.format

Then, to help users migrate pants.backend.experimental.python.lint.ruff can just be a "reexport" of the rules from pants.backend.experimental.python.lint.ruff.check, but with a deprecation.

I'm trying to think to "how would we do it if were just adding a ruff backend now", and I'm not sure we'd "prioritise" ruff check over ruff format? But maybe we would, I don't know! Thoughts?

I like using subpackages under python.lint.ruff since these are very related backends.

@krishnan-chandra
Copy link
Contributor Author

krishnan-chandra commented Feb 15, 2024

I'm trying to think to "how would we do it if were just adding a ruff backend now", and I'm not sure we'd "prioritise" ruff check over ruff format? But maybe we would, I don't know! Thoughts?

I don't think there's any priority order between the tools per se - the only reason the packages are structured the way they are is for backwards compatibility and because the Ruff checker existed first. I'm happy to organize them under a single parent package as you suggest

@kaos
Copy link
Member

kaos commented Feb 15, 2024

+1 for @huonw's suggestion :)

@krishnan-chandra
Copy link
Contributor Author

Also verified that this works with both new backends enabled:

backend_packages = [
  # Python
  "pants.backend.python",
  "pants.backend.python.lint.autoflake",
  "pants.backend.experimental.python.lint.ruff.check",
  "pants.backend.experimental.python.lint.ruff.format",
  "pants.backend.experimental.python.typecheck.pyright",
]
08:35:31.53 [INFO] Completed: Format with shfmt - shfmt made no changes.
08:35:33.24 [INFO] Completed: Fix with Autoflake - autoflake made no changes.
08:35:33.27 [INFO] Completed: Fix with `ruff check --fix` - ruff check --fix made no changes.
08:35:33.57 [INFO] Completed: Format with `ruff format` - ruff format made no changes.

Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

🚀

@kaos
Copy link
Member

kaos commented Feb 15, 2024

LGTM. 🕐 more 👀 -then> 🚢 :)

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Just one small issue I noticed. Otherwise, this looks great!

Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Python Python backend-related issues category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrading to 2.20 with ruff linting active immediately starts using it for formatting too
4 participants