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
Harmonize help commands' --format
to --output-format
with deprecation warnings
#8203
Conversation
Nice. I linked the related issue. Would it be possible to log a warning when using |
Ah, I didn't even know that there was an issue about this 😁 Sure, I can canonicalize it all to |
PR Check ResultsEcosystem✅ ecosystem check detected no linter changes. |
I think that's fine, especially considering that it used to be the same before? @zanieb what's your take? |
--format
to --output-format
with deprecation warnings
@MichaReiser Done deal ✨ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a bit more discussion before it goes out.
I think it's okay for a subset of formats to be supported on the CLI, but it doesn't really make sense when using the environment variable. Like.. if you've set your output format in your environment to something not supported by another command it'll just fail?
In combination with that concern, it seems problematic to add support for RUFF_FORMAT
to commands to that did not previously have it as all use of those commands could start to fail.
In general, I'm also not sure it makes sense to add support for deprecated options to new commands i.e. adding --format
support to ruff version
.
Can you expand on this? |
Well we have the output formats supported by And then we have the output formats supported by "help" commands: text, json If we fail on unknown formats and use
I don't think the help formats should use the same variable at least, and maybe they shouldn't support environment variables at all. |
Hmm I see. How about leaving the |
…tion warnings Warnings borrowed from astral-sh#7514 Fixes astral-sh#7990
@charliermarsh @zanieb Done: removed the envvars, removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making the changes! I have some minor comments.
crates/ruff_cli/src/lib.rs
Outdated
format, | ||
mut output_format, | ||
} => { | ||
output_format = warn_about_deprecated_help_format(output_format, format); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps resolve_help_output_format
would be a better function name? No strong feelings though.
Co-authored-by: Zanie Blue <contact@zanie.dev>
Thank you as always @akx! |
Summary
Since
--format
was changed to--output-format
forcheck
, it feels like it makes sense for the same to work for the auxiliary commands.This
format
option tooutput-format
#7514 (and un-became a thing in Remove support for providing output format viaformat
option #7984)Fixes #7990.
Test Plan
cargo run --bin=ruff -- rule --all --output-format=json
workscargo run --bin=ruff -- rule --format=json
works with warnings