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

Harmonize help commands' --format to --output-format with deprecation warnings #8203

Merged
merged 5 commits into from Oct 28, 2023

Conversation

akx
Copy link
Contributor

@akx akx commented Oct 25, 2023

Summary

Since --format was changed to --output-format for check, it feels like it makes sense for the same to work for the auxiliary commands.

This

Fixes #7990.

Test Plan

  • cargo run --bin=ruff -- rule --all --output-format=json works
  • cargo run --bin=ruff -- rule --format=json works with warnings

@MichaReiser MichaReiser added the cli Related to the command-line interface label Oct 25, 2023
@MichaReiser
Copy link
Member

Nice. I linked the related issue. Would it be possible to log a warning when using --format that mention that it is deprecated in favor of --output-format?

@akx
Copy link
Contributor Author

akx commented Oct 25, 2023

Ah, I didn't even know that there was an issue about this 😁

Sure, I can canonicalize it all to output_format and add warnings, but will it be weird that the help commands support a subset of the main check output formats with the same flag name?

@github-actions
Copy link

github-actions bot commented Oct 25, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no linter changes.

@MichaReiser
Copy link
Member

Sure, I can canonicalize it all to output_format and add warnings, but will it be weird that the help commands support a subset of the main check output formats with the same flag name?

I think that's fine, especially considering that it used to be the same before? @zanieb what's your take?

@akx akx changed the title Support --output-format as alias for --format for help commands Harmonize help commands' --format to --output-format with deprecation warnings Oct 25, 2023
@akx
Copy link
Contributor Author

akx commented Oct 25, 2023

@MichaReiser Done deal ✨

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.

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.

@charliermarsh
Copy link
Member

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?

Can you expand on this?

@zanieb
Copy link
Member

zanieb commented Oct 25, 2023

Well we have the output formats supported by ruff check: text, json, json-lines, junit, grouped, github, gitlab, pylint, azure

And then we have the output formats supported by "help" commands: text, json

If we fail on unknown formats and use RUFF_FORMAT/RUFF_OUTPUT_FORMAT for both of these I believe this will break existing usage and be confusing for future users.

export RUFF_OUTPUT_FORMAT=github
ruff check    # OK
ruff version  # FAILS with invalid output format

I don't think the help formats should use the same variable at least, and maybe they shouldn't support environment variables at all.

@charliermarsh
Copy link
Member

Hmm I see. How about leaving the --format and --output-format support for these commands (for consistency, for now -- we'd then deprecate --format for all of them at once), but omitting the environment variable support?

@akx
Copy link
Contributor Author

akx commented Oct 26, 2023

@charliermarsh @zanieb Done: removed the envvars, removed --format from version (which hadn't had it in the first place).

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.

Thank you for making the changes! I have some minor comments.

crates/ruff_cli/src/lib.rs Outdated Show resolved Hide resolved
crates/ruff_cli/src/lib.rs Show resolved Hide resolved
format,
mut output_format,
} => {
output_format = warn_about_deprecated_help_format(output_format, format);
Copy link
Member

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.

akx and others added 2 commits October 26, 2023 19:45
@charliermarsh charliermarsh enabled auto-merge (squash) October 28, 2023 03:26
@charliermarsh
Copy link
Member

Thank you as always @akx!

@charliermarsh charliermarsh merged commit 7b4b004 into astral-sh:main Oct 28, 2023
14 checks passed
@akx akx deleted the harmonize-format branch October 29, 2023 13:24
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.

Rename use of --format in ruff rule to --output-format
4 participants