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

Replace --show-source and --no-show-source with --output_format=<full|concise> #9687

Merged
merged 10 commits into from
Jan 30, 2024
53 changes: 46 additions & 7 deletions crates/ruff/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use ruff_linter::settings::types::{
ExtensionPair, FilePattern, PatternPrefixPair, PerFileIgnore, PreviewMode, PythonVersion,
SerializationFormat, UnsafeFixes,
};
use ruff_linter::{RuleParser, RuleSelector, RuleSelectorParser};
use ruff_linter::{warn_user, RuleParser, RuleSelector, RuleSelectorParser};
use ruff_workspace::configuration::{Configuration, RuleSelection};
use ruff_workspace::options::PycodestyleOptions;
use ruff_workspace::resolver::ConfigurationTransformer;
Expand Down Expand Up @@ -104,6 +104,7 @@ pub struct CheckCommand {
no_unsafe_fixes: bool,
/// Show violations with source code.
/// Use `--no-show-source` to disable.
/// (Deprecated: use `--output-format=full` or `--output-format=concise` instead of `--show-source` and `--no-show-source`, respectively)
#[arg(long, overrides_with("no_show_source"))]
show_source: bool,
#[clap(long, overrides_with("show_source"), hide = true)]
Expand Down Expand Up @@ -131,6 +132,8 @@ pub struct CheckCommand {
ignore_noqa: bool,

/// Output serialization format for violations.
/// The default serialization format is "concise".
snowsignal marked this conversation as resolved.
Show resolved Hide resolved
/// In preview mode, the default serialization format is "full".
#[arg(long, value_enum, env = "RUFF_OUTPUT_FORMAT")]
pub output_format: Option<SerializationFormat>,

Expand Down Expand Up @@ -533,7 +536,6 @@ impl CheckCommand {
self.no_respect_gitignore,
),
select: self.select,
show_source: resolve_bool_arg(self.show_source, self.no_show_source),
target_version: self.target_version,
unfixable: self.unfixable,
// TODO(charlie): Included in `pyproject.toml`, but not inherited.
Expand All @@ -543,7 +545,11 @@ impl CheckCommand {
unsafe_fixes: resolve_bool_arg(self.unsafe_fixes, self.no_unsafe_fixes)
.map(UnsafeFixes::from),
force_exclude: resolve_bool_arg(self.force_exclude, self.no_force_exclude),
output_format: self.output_format,
output_format: resolve_output_format(
self.output_format,
resolve_bool_arg(self.show_source, self.no_show_source),
resolve_bool_arg(self.preview, self.no_preview).unwrap_or_default(),
),
show_fixes: resolve_bool_arg(self.show_fixes, self.no_show_fixes),
extension: self.extension,
},
Expand Down Expand Up @@ -594,6 +600,43 @@ fn resolve_bool_arg(yes: bool, no: bool) -> Option<bool> {
}
}

fn resolve_output_format(
output_format: Option<SerializationFormat>,
show_sources: Option<bool>,
preview: bool,
) -> Option<SerializationFormat> {
Some(match (output_format, show_sources) {
(Some(o), None) => o,
(Some(SerializationFormat::Grouped), Some(true)) => {
warn_user!("`--show-source` with `--output-format=grouped` is deprecated, and will not show source files. Use `--output-format=full` to show source information.");
SerializationFormat::Grouped
}
(Some(fmt), Some(true)) => {
warn_user!("The `--show-source` argument is deprecated and has been ignored in favor of `--output-format={fmt}`.");
fmt
}
(Some(fmt), Some(false)) => {
warn_user!("The `--no-show-source` argument is deprecated and has been ignored in favor of `--output-format={fmt}`.");
fmt
}
(None, Some(true)) => {
warn_user!("The `--show-source` argument is deprecated. Use `--output-format=full` instead.");
SerializationFormat::Full
}
(None, Some(false)) => {
warn_user!("The `--no-show-source` argument is deprecated. Use `--output-format=concise` instead.");
SerializationFormat::Concise
}
(None, None) => return None
}).map(|format| match format {
SerializationFormat::Text => {
warn_user!("`--output-format=text` is deprecated. Use `--output-format=full` or `--output-format=concise` instead. `text` will be treated as `{}`.", SerializationFormat::default(preview));
SerializationFormat::default(preview)
},
other => other
})
}
snowsignal marked this conversation as resolved.
Show resolved Hide resolved

/// CLI settings that are distinct from configuration (commands, lists of files,
/// etc.).
#[allow(clippy::struct_excessive_bools)]
Expand Down Expand Up @@ -648,7 +691,6 @@ pub struct CliOverrides {
pub preview: Option<PreviewMode>,
pub respect_gitignore: Option<bool>,
pub select: Option<Vec<RuleSelector>>,
pub show_source: Option<bool>,
pub target_version: Option<PythonVersion>,
pub unfixable: Option<Vec<RuleSelector>>,
// TODO(charlie): Captured in pyproject.toml as a default, but not part of `Settings`.
Expand Down Expand Up @@ -735,9 +777,6 @@ impl ConfigurationTransformer for CliOverrides {
if let Some(respect_gitignore) = &self.respect_gitignore {
config.respect_gitignore = Some(*respect_gitignore);
}
if let Some(show_source) = &self.show_source {
config.show_source = Some(*show_source);
}
if let Some(show_fixes) = &self.show_fixes {
config.show_fixes = Some(*show_fixes);
}
Expand Down
17 changes: 9 additions & 8 deletions crates/ruff/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,6 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
unsafe_fixes,
output_format,
show_fixes,
show_source,
..
} = pyproject_config.settings;

Expand Down Expand Up @@ -284,9 +283,6 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
if show_fixes {
printer_flags |= PrinterFlags::SHOW_FIX_SUMMARY;
}
if show_source {
printer_flags |= PrinterFlags::SHOW_SOURCE;
}
if cli.ecosystem_ci {
warn_user!(
"The formatting of fixes emitted by this option is a work-in-progress, subject to \
Expand Down Expand Up @@ -325,9 +321,14 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
printer_flags,
);

let preview = overrides.preview.unwrap_or_default().is_enabled();
Copy link
Member

Choose a reason for hiding this comment

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

Does this respect preview = true in configuration files? I think this might only respect --preview when it's set on the command line -- is that deliberate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No that was not deliberate, good catch.

Copy link
Member

Choose a reason for hiding this comment

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

No worries! Was trying to pin down the build failures on #9599 after merging in main, and spotted this :)


if cli.watch {
if output_format != SerializationFormat::Text {
warn_user!("`--output-format text` is always used in watch mode.");
if output_format != SerializationFormat::default(preview) {
warn_user!(
"`--output-format {}` is always used in watch mode.",
SerializationFormat::default(preview)
);
}

// Configure the file watcher.
Expand All @@ -353,7 +354,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
fix_mode,
unsafe_fixes,
)?;
printer.write_continuously(&mut writer, &messages)?;
printer.write_continuously(&mut writer, &messages, preview)?;

// In watch mode, we may need to re-resolve the configuration.
// TODO(charlie): Re-compute other derivative values, like the `printer`.
Expand Down Expand Up @@ -386,7 +387,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
fix_mode,
unsafe_fixes,
)?;
printer.write_continuously(&mut writer, &messages)?;
printer.write_continuously(&mut writer, &messages, preview)?;
}
Err(err) => return Err(err.into()),
}
Expand Down
21 changes: 13 additions & 8 deletions crates/ruff/src/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ bitflags! {
pub(crate) struct Flags: u8 {
/// Whether to show violations when emitting diagnostics.
const SHOW_VIOLATIONS = 0b0000_0001;
/// Whether to show the source code when emitting diagnostics.
const SHOW_SOURCE = 0b000_0010;
/// Whether to show a summary of the fixed violations when emitting diagnostics.
const SHOW_FIX_SUMMARY = 0b0000_0100;
/// Whether to show a diff of each fixed violation when emitting diagnostics.
Expand Down Expand Up @@ -218,7 +216,10 @@ impl Printer {
if !self.flags.intersects(Flags::SHOW_VIOLATIONS) {
if matches!(
self.format,
SerializationFormat::Text | SerializationFormat::Grouped
SerializationFormat::Text
| SerializationFormat::Full
| SerializationFormat::Concise
| SerializationFormat::Grouped
) {
if self.flags.intersects(Flags::SHOW_FIX_SUMMARY) {
if !diagnostics.fixed.is_empty() {
Expand All @@ -245,11 +246,12 @@ impl Printer {
SerializationFormat::Junit => {
JunitEmitter.emit(writer, &diagnostics.messages, &context)?;
}
SerializationFormat::Text => {
SerializationFormat::Concise
| SerializationFormat::Full => {
TextEmitter::default()
.with_show_fix_status(show_fix_status(self.fix_mode, fixables.as_ref()))
.with_show_fix_diff(self.flags.intersects(Flags::SHOW_FIX_DIFF))
.with_show_source(self.flags.intersects(Flags::SHOW_SOURCE))
.with_show_source(self.format == SerializationFormat::Full)
.with_unsafe_fixes(self.unsafe_fixes)
.emit(writer, &diagnostics.messages, &context)?;

Expand All @@ -265,7 +267,6 @@ impl Printer {
}
SerializationFormat::Grouped => {
GroupedEmitter::default()
.with_show_source(self.flags.intersects(Flags::SHOW_SOURCE))
.with_show_fix_status(show_fix_status(self.fix_mode, fixables.as_ref()))
.with_unsafe_fixes(self.unsafe_fixes)
.emit(writer, &diagnostics.messages, &context)?;
Expand Down Expand Up @@ -294,6 +295,7 @@ impl Printer {
SerializationFormat::Sarif => {
SarifEmitter.emit(writer, &diagnostics.messages, &context)?;
}
SerializationFormat::Text => unreachable!("Text is deprecated and should have been automatically converted to the default serialization format")
}

writer.flush()?;
Expand Down Expand Up @@ -342,7 +344,9 @@ impl Printer {
}

match self.format {
SerializationFormat::Text => {
SerializationFormat::Text
| SerializationFormat::Full
| SerializationFormat::Concise => {
// Compute the maximum number of digits in the count and code, for all messages,
// to enable pretty-printing.
let count_width = num_digits(
Expand Down Expand Up @@ -403,6 +407,7 @@ impl Printer {
&self,
writer: &mut dyn Write,
diagnostics: &Diagnostics,
preview: bool,
) -> Result<()> {
if matches!(self.log_level, LogLevel::Silent) {
return Ok(());
Expand Down Expand Up @@ -430,7 +435,7 @@ impl Printer {
let context = EmitterContext::new(&diagnostics.notebook_indexes);
TextEmitter::default()
.with_show_fix_status(show_fix_status(self.fix_mode, fixables.as_ref()))
.with_show_source(self.flags.intersects(Flags::SHOW_SOURCE))
.with_show_source(preview)
.with_unsafe_fixes(self.unsafe_fixes)
.emit(writer, &diagnostics.messages, &context)?;
}
Expand Down
149 changes: 149 additions & 0 deletions crates/ruff/tests/deprecation.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
//! A test suite that ensures deprecated command line options have appropriate warnings / behaviors

use ruff_linter::settings::types::SerializationFormat;
use std::process::Command;

use insta_cmd::{assert_cmd_snapshot, get_cargo_bin};

const BIN_NAME: &str = "ruff";

const STDIN: &str = "l = 1";

fn ruff_check(show_source: Option<bool>, output_format: Option<String>) -> Command {
let mut cmd = Command::new(get_cargo_bin(BIN_NAME));
let output_format = output_format.unwrap_or(format!("{}", SerializationFormat::default(false)));
cmd.arg("--output-format");
cmd.arg(output_format);
cmd.arg("--no-cache");
match show_source {
Some(true) => {
cmd.arg("--show-source");
}
Some(false) => {
cmd.arg("--no-show-source");
}
None => {}
}
cmd.arg("-");

cmd
}

#[test]
fn ensure_show_source_is_deprecated() {
assert_cmd_snapshot!(ruff_check(Some(true), None).pass_stdin(STDIN), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:1: E741 Ambiguous variable name: `l`
Found 1 error.

----- stderr -----
warning: The `--show-source` argument is deprecated and has been ignored in favor of `--output-format=concise`.
"###);
}

#[test]
fn ensure_no_show_source_is_deprecated() {
assert_cmd_snapshot!(ruff_check(Some(false), None).pass_stdin(STDIN), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:1: E741 Ambiguous variable name: `l`
Found 1 error.

----- stderr -----
warning: The `--no-show-source` argument is deprecated and has been ignored in favor of `--output-format=concise`.
"###);
}

#[test]
fn ensure_output_format_is_deprecated() {
assert_cmd_snapshot!(ruff_check(None, Some("text".into())).pass_stdin(STDIN), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:1: E741 Ambiguous variable name: `l`
Found 1 error.

----- stderr -----
warning: `--output-format=text` is deprecated. Use `--output-format=full` or `--output-format=concise` instead. `text` will be treated as `concise`.
"###);
}

#[test]
fn ensure_output_format_overrides_show_source() {
assert_cmd_snapshot!(ruff_check(Some(true), Some("concise".into())).pass_stdin(STDIN), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:1: E741 Ambiguous variable name: `l`
Found 1 error.

----- stderr -----
warning: The `--show-source` argument is deprecated and has been ignored in favor of `--output-format=concise`.
"###);
}

#[test]
fn ensure_full_output_format_overrides_no_show_source() {
assert_cmd_snapshot!(ruff_check(Some(false), Some("full".into())).pass_stdin(STDIN), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:1: E741 Ambiguous variable name: `l`
|
1 | l = 1
| ^ E741
|

Found 1 error.

----- stderr -----
warning: The `--no-show-source` argument is deprecated and has been ignored in favor of `--output-format=full`.
"###);
}

#[test]
fn ensure_output_format_uses_concise_over_no_show_source() {
assert_cmd_snapshot!(ruff_check(Some(false), Some("concise".into())).pass_stdin(STDIN), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:1: E741 Ambiguous variable name: `l`
Found 1 error.

----- stderr -----
warning: The `--no-show-source` argument is deprecated and has been ignored in favor of `--output-format=concise`.
"###);
}

#[test]
fn ensure_deprecated_output_format_overrides_show_source() {
assert_cmd_snapshot!(ruff_check(Some(true), Some("text".into())).pass_stdin(STDIN), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:1: E741 Ambiguous variable name: `l`
Found 1 error.

----- stderr -----
warning: The `--show-source` argument is deprecated and has been ignored in favor of `--output-format=text`.
warning: `--output-format=text` is deprecated. Use `--output-format=full` or `--output-format=concise` instead. `text` will be treated as `concise`.
"###);
}

#[test]
fn ensure_deprecated_output_format_overrides_no_show_source() {
assert_cmd_snapshot!(ruff_check(Some(false), Some("text".into())).pass_stdin(STDIN), @r###"
success: false
exit_code: 1
----- stdout -----
-:1:1: E741 Ambiguous variable name: `l`
Found 1 error.

----- stderr -----
warning: The `--no-show-source` argument is deprecated and has been ignored in favor of `--output-format=text`.
warning: `--output-format=text` is deprecated. Use `--output-format=full` or `--output-format=concise` instead. `text` will be treated as `concise`.
"###);
}
Loading
Loading