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

Remove output format text and use format full by default #12010

Merged
merged 3 commits into from
Jun 26, 2024

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jun 24, 2024

Summary

This PR removes support for the output format text in favor of concise and makes full the new default (it was already the default for preview mode). The OutputFormat::Text is still defined in code so that we can error when it's being used.

Resolves #7349

Test Plan

I tested that using --output-format on the CLI errors:

❯ cargo run --bin ruff -- check --output-format text
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.12s
     Running `target/debug/ruff check --output-format text`
ruff failed
  Cause: `--output-format=text` is deprecated. Use `--output-format=full` or `--output-format=concise` instead.

❯ cargo run --bin ruff -- check --config="output-format='text'"
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.12s
     Running `target/debug/ruff check '--config=output-format='\''text'\'''`
ruff failed
  Cause: The Setting `output_format=text` has been deprecated. Update your `--config` CLI arguments to use `output-format="concise"` or  `output-format="full"` instead.

I verified that using output-format="text" in the settings errors

❯ ../ruff/target/debug/ruff check test.py
ruff failed
  Cause: The Setting `output_format=text` has been deprecated. Update `pyproject.toml` to use `output-format="concise"` or  `output-format="full"` instead.
  • I verified that using concise in the settings or on the CLI works as expected
  • I verified that ruff defaults to full when no output-format is specified on the CLI or in the settings.

@MichaReiser MichaReiser changed the title Remove output format text and use format full by defualt Remove output format text and use format full by default Jun 24, 2024
@MichaReiser MichaReiser changed the base branch from main to ruff-0.5 June 24, 2024 09:04
@MichaReiser MichaReiser requested review from zanieb and snowsignal June 24, 2024 09:05
@MichaReiser MichaReiser added breaking Breaking API change configuration Related to settings and configuration cli Related to the command-line interface labels Jun 24, 2024
Comment on lines +1301 to +1223
|
|
Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like a bug but it isn't. Or, it is a bug in the test rule that always emits an empty range instead of emitting an actual range.

@MichaReiser MichaReiser added this to the v0.5.0 milestone Jun 24, 2024
Copy link
Contributor

github-actions bot commented Jun 24, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@zanieb
Copy link
Member

zanieb commented Jun 24, 2024

I'm a little worried about making this change when the full format is half-complete (#7352)

@MichaReiser
Copy link
Member Author

@zanieb what's the part that you're worried about?

I think showing the code frame for diagnostics is already an improvement by itself worth shipping and the format name full is generic enough to allow us to also show fixes in a future version.

@zanieb
Copy link
Member

zanieb commented Jun 24, 2024

It just doesn't feel as useful over the concise output without the fixes being shown (seen feedback from one user about this). Oh well though.

@MichaReiser
Copy link
Member Author

I personally find the source text in the rust compiler extremely useful. I don't think I ever look at the line number to get context on where I need to fix something.

Copy link
Contributor

@snowsignal snowsignal left a comment

Choose a reason for hiding this comment

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

LGTM - it might be nice to have a small test plan :)

@MichaReiser MichaReiser force-pushed the output-format-full-by-default branch 2 times, most recently from 7aa60a4 to 9bb1ccc Compare June 25, 2024 08:15
@MichaReiser MichaReiser marked this pull request as ready for review June 25, 2024 08:15
@MichaReiser
Copy link
Member Author

I updated the PR to keep the Text option for now but I made the usage of Text a hard error.

crates/ruff/src/args.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser force-pushed the output-format-full-by-default branch from 9bb1ccc to b8d1ba6 Compare June 26, 2024 06:54
@MichaReiser MichaReiser merged commit 0501afd into ruff-0.5 Jun 26, 2024
20 checks passed
@MichaReiser MichaReiser deleted the output-format-full-by-default branch June 26, 2024 07:40
This was referenced Jun 26, 2024
MichaReiser added a commit that referenced this pull request Jun 27, 2024
MichaReiser added a commit that referenced this pull request Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking API change cli Related to the command-line interface configuration Related to settings and configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants