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 --exit-non-zero-on-format #16009

Merged
merged 4 commits into from
Mar 19, 2025
Merged

Conversation

thejcannon
Copy link
Contributor

Summary

Fixes #8191 by introducing --exit-non-zero-on-format to ruff format which pretty much does what it says on the tin.

Test Plan

Added a new test!

Copy link
Contributor

github-actions bot commented Feb 7, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser MichaReiser requested a review from zanieb February 7, 2025 06:44
@MichaReiser MichaReiser added the cli Related to the command-line interface label Feb 7, 2025
Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines 532 to 533
#[arg(long, help_heading = "Miscellaneous")]
pub exit_non_zero_on_format: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Do you find ruff format --exit-non-zero-on-format awkward? Should it be ruff format --exit-non-zero-on-change or --exit-non-zero-on-write? It might be nice to switch to an agnostic name.

Should we retain the on_fix language we use for ruff check? I see the motivation for not, but at the very least we probably want an alias for parity with ruff check?

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 current name was meant to "reflect" "exit non zero on (action)", to mirror "on fix".

As for the ergonomics? I really don't have an opinion. I can't imagine anyone using this flag outside of some kind of config file or script file of some sort. In which case the name is really only as important as "can be discovered" and "name reflects what happens". So I think the bike shed on the name (past probably "exit non zero on ...") doesn't bother me. I'm happy to use whatever yall like most!

@zanieb
Copy link
Member

zanieb commented Feb 8, 2025

(On second thought, I do want to talk briefly about the name before merging)

@thejcannon
Copy link
Contributor Author

@zanieb bump?

@zanieb
Copy link
Member

zanieb commented Mar 17, 2025

Thanks for the bump, sorry about the delay. Let me prod some others on painting this bikeshed.

@thejcannon
Copy link
Contributor Author

No worries! My hobby project exploring lefthook just uses my fork.

But this week I decided to make another hobby project and remembered this issue 😁

@ntBre
Copy link
Contributor

ntBre commented Mar 19, 2025

Thanks again for your work on this! After discussing internally, we liked your --exit-non-zero-on-format flag name but wanted to add an alias for --exit-non-zero-on-fix to match the linter. I just made that change and duplicated your tests with the new alias.

@ntBre ntBre merged commit 8619317 into astral-sh:main Mar 19, 2025
22 checks passed
@thejcannon thejcannon deleted the exit-non-zero-on-format branch March 19, 2025 17:01
dcreager added a commit that referenced this pull request Mar 21, 2025
* main: (26 commits)
  Use the common `OperatorPrecedence` for the parser (#16747)
  [red-knot] Check subtype relation between callable types (#16804)
  [red-knot] Check whether two callable types are equivalent (#16698)
  [red-knot] Ban most `Type::Instance` types in type expressions (#16872)
  Special-case value-expression inference of special form subscriptions (#16877)
  [syntax-errors] Fix star annotation before Python 3.11 (#16878)
  Recognize `SyntaxError:` as an error code for ecosystem checks (#16879)
  [red-knot] add test cases result in false positive errors (#16856)
  Bump 0.11.1 (#16871)
  Allow discovery of venv in VIRTUAL_ENV env variable (#16853)
  Split git pathspecs in change determination onto separate lines (#16869)
  Use the correct base commit for change determination (#16857)
  Separate `BitXorOr` into `BitXor` and `BitOr` precedence (#16844)
  Server: Allow `FixAll` action in presence of version-specific syntax errors (#16848)
  [`refurb`] Fix starred expressions fix (`FURB161`) (#16550)
  [`flake8-executable`] Add pytest and uv run to help message for `shebang-missing-python` (`EXE003`) (#16855)
  Show more precise messages in invalid type expressions (#16850)
  [`flake8-executables`] Allow `uv run` in shebang line for `shebang-missing-python` (`EXE003`) (#16849)
  Add `--exit-non-zero-on-format` (#16009)
  [red-knot] Ban list literals in most contexts in type expressions (#16847)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command-line interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an --exit-non-zero-on-fix equivalent to the formatter.
4 participants