Add option (-o/--output-file) to write output to a file (#4950)

## Summary

A new CLI option (`-o`/`--output-file`) to write output to a file
instead of stdout.

Major change is to remove the lock acquired on stdout. The argument is
that the output is buffered and thus the lock is acquired only when
writing a block (8kb). As per the benchmark below there is a slight
performance penalty.

Reference:
https://rustmagazine.org/issue-3/javascript-compiler/#printing-is-slow

## Benchmarks

_Output is truncated to only contain useful information:_

Command: `check --isolated --no-cache --select=ALL --show-source
./test-repos/cpython"`

Latest HEAD (361d45f2b2) with and without
the manual lock on stdout:

```console
Benchmark 1: With lock
  Time (mean ± σ):      5.687 s ±  0.075 s    [User: 17.110 s, System: 0.486 s]
  Range (min … max):    5.615 s …  5.860 s    10 runs

Benchmark 2: Without lock
  Time (mean ± σ):      5.719 s ±  0.064 s    [User: 17.095 s, System: 0.491 s]
  Range (min … max):    5.640 s …  5.865 s    10 runs

Summary
  (1) ran 1.01 ± 0.02 times faster than (2)
```

This PR:

```console
Benchmark 1: This PR
  Time (mean ± σ):      5.855 s ±  0.058 s    [User: 17.197 s, System: 0.491 s]
  Range (min … max):    5.786 s …  5.987 s    10 runs
 
Benchmark 2: Latest HEAD with lock
  Time (mean ± σ):      5.645 s ±  0.033 s    [User: 16.922 s, System: 0.495 s]
  Range (min … max):    5.600 s …  5.712 s    10 runs
 
Summary
  (2) ran 1.04 ± 0.01 times faster than (1)
```

## Test Plan

Run all of the commands which gives output with and without the
`--output-file=ruff.out` option:
* `--show-settings`
* `--show-files`
* `--show-fixes`
* `--diff`
* `--select=ALL`
* `--select=All --show-source`
* `--watch` (only stdout allowed)

resolves: #4754
This commit is contained in:
Dhruv Manilawala 2023-06-20 22:16:49 +05:30 committed by GitHub
parent d9e59b21cd
commit 6f7d3cc798
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 64 additions and 38 deletions

View file

@ -107,6 +107,9 @@ pub struct CheckArgs {
/// Output serialization format for violations.
#[arg(long, value_enum, env = "RUFF_FORMAT")]
pub format: Option<SerializationFormat>,
/// Specify file to write the linter output to (default: stdout).
#[arg(short, long)]
pub output_file: Option<PathBuf>,
/// The minimum Python version that should be supported.
#[arg(long, value_enum)]
pub target_version: Option<PythonVersion>,
@ -399,6 +402,7 @@ impl CheckArgs {
ignore_noqa: self.ignore_noqa,
isolated: self.isolated,
no_cache: self.no_cache,
output_file: self.output_file,
show_files: self.show_files,
show_settings: self.show_settings,
statistics: self.statistics,
@ -465,6 +469,7 @@ pub struct Arguments {
pub ignore_noqa: bool,
pub isolated: bool,
pub no_cache: bool,
pub output_file: Option<PathBuf>,
pub show_files: bool,
pub show_settings: bool,
pub statistics: bool,

View file

@ -1,4 +1,4 @@
use std::io::{self, BufWriter, Write};
use std::io::Write;
use std::path::PathBuf;
use anyhow::Result;
@ -14,6 +14,7 @@ pub(crate) fn show_files(
files: &[PathBuf],
pyproject_config: &PyprojectConfig,
overrides: &Overrides,
writer: &mut impl Write,
) -> Result<()> {
// Collect all files in the hierarchy.
let (paths, _resolver) = resolver::python_files_in_path(files, pyproject_config, overrides)?;
@ -24,13 +25,12 @@ pub(crate) fn show_files(
}
// Print the list of files.
let mut stdout = BufWriter::new(io::stdout().lock());
for entry in paths
.iter()
.flatten()
.sorted_by(|a, b| a.path().cmp(b.path()))
{
writeln!(stdout, "{}", entry.path().to_string_lossy())?;
writeln!(writer, "{}", entry.path().to_string_lossy())?;
}
Ok(())

View file

@ -1,4 +1,4 @@
use std::io::{self, BufWriter, Write};
use std::io::Write;
use std::path::PathBuf;
use anyhow::{bail, Result};
@ -14,6 +14,7 @@ pub(crate) fn show_settings(
files: &[PathBuf],
pyproject_config: &PyprojectConfig,
overrides: &Overrides,
writer: &mut impl Write,
) -> Result<()> {
// Collect all files in the hierarchy.
let (paths, resolver) = resolver::python_files_in_path(files, pyproject_config, overrides)?;
@ -28,12 +29,11 @@ pub(crate) fn show_settings(
let path = entry.path();
let settings = resolver.resolve(path, pyproject_config);
let mut stdout = BufWriter::new(io::stdout().lock());
writeln!(stdout, "Resolved settings for: {path:?}")?;
writeln!(writer, "Resolved settings for: {path:?}")?;
if let Some(settings_path) = pyproject_config.path.as_ref() {
writeln!(stdout, "Settings path: {settings_path:?}")?;
writeln!(writer, "Settings path: {settings_path:?}")?;
}
writeln!(stdout, "{settings:#?}")?;
writeln!(writer, "{settings:#?}")?;
Ok(())
}

View file

@ -1,3 +1,4 @@
use std::fs::File;
use std::io::{self, stdout, BufWriter, Write};
use std::path::{Path, PathBuf};
use std::process::ExitCode;
@ -171,12 +172,26 @@ pub fn check(args: CheckArgs, log_level: LogLevel) -> Result<ExitStatus> {
cli.stdin_filename.as_deref(),
)?;
let mut writer: Box<dyn Write> = match cli.output_file {
Some(path) if !cli.watch => {
colored::control::set_override(false);
let file = File::create(path)?;
Box::new(BufWriter::new(file))
}
_ => Box::new(BufWriter::new(io::stdout())),
};
if cli.show_settings {
commands::show_settings::show_settings(&cli.files, &pyproject_config, &overrides)?;
commands::show_settings::show_settings(
&cli.files,
&pyproject_config,
&overrides,
&mut writer,
)?;
return Ok(ExitStatus::Success);
}
if cli.show_files {
commands::show_files::show_files(&cli.files, &pyproject_config, &overrides)?;
commands::show_files::show_files(&cli.files, &pyproject_config, &overrides, &mut writer)?;
return Ok(ExitStatus::Success);
}
@ -276,7 +291,7 @@ pub fn check(args: CheckArgs, log_level: LogLevel) -> Result<ExitStatus> {
noqa.into(),
autofix,
)?;
printer.write_continuously(&messages)?;
printer.write_continuously(&mut writer, &messages)?;
// In watch mode, we may need to re-resolve the configuration.
// TODO(charlie): Re-compute other derivative values, like the `printer`.
@ -308,7 +323,7 @@ pub fn check(args: CheckArgs, log_level: LogLevel) -> Result<ExitStatus> {
noqa.into(),
autofix,
)?;
printer.write_continuously(&messages)?;
printer.write_continuously(&mut writer, &messages)?;
}
Err(err) => return Err(err.into()),
}
@ -341,10 +356,9 @@ pub fn check(args: CheckArgs, log_level: LogLevel) -> Result<ExitStatus> {
// source code goes to stdout).
if !(is_stdin && matches!(autofix, flags::FixMode::Apply | flags::FixMode::Diff)) {
if cli.statistics {
printer.write_statistics(&diagnostics)?;
printer.write_statistics(&diagnostics, &mut writer)?;
} else {
let mut stdout = BufWriter::new(io::stdout().lock());
printer.write_once(&diagnostics, &mut stdout)?;
printer.write_once(&diagnostics, &mut writer)?;
}
}

View file

@ -1,8 +1,7 @@
use std::cmp::Reverse;
use std::fmt::Display;
use std::hash::Hash;
use std::io;
use std::io::{BufWriter, Write};
use std::io::Write;
use anyhow::Result;
use bitflags::bitflags;
@ -98,7 +97,7 @@ impl Printer {
}
}
fn write_summary_text(&self, stdout: &mut dyn Write, diagnostics: &Diagnostics) -> Result<()> {
fn write_summary_text(&self, writer: &mut dyn Write, diagnostics: &Diagnostics) -> Result<()> {
if self.log_level >= LogLevel::Default {
if self.flags.contains(Flags::SHOW_VIOLATIONS) {
let fixed = diagnostics
@ -111,12 +110,12 @@ impl Printer {
if fixed > 0 {
let s = if total == 1 { "" } else { "s" };
writeln!(
stdout,
writer,
"Found {total} error{s} ({fixed} fixed, {remaining} remaining)."
)?;
} else if remaining > 0 {
let s = if remaining == 1 { "" } else { "s" };
writeln!(stdout, "Found {remaining} error{s}.")?;
writeln!(writer, "Found {remaining} error{s}.")?;
}
if show_fix_status(self.autofix_level) {
@ -127,7 +126,7 @@ impl Printer {
.count();
if num_fixable > 0 {
writeln!(
stdout,
writer,
"[{}] {num_fixable} potentially fixable with the --fix option.",
"*".cyan(),
)?;
@ -142,9 +141,9 @@ impl Printer {
if fixed > 0 {
let s = if fixed == 1 { "" } else { "s" };
if self.autofix_level.is_apply() {
writeln!(stdout, "Fixed {fixed} error{s}.")?;
writeln!(writer, "Fixed {fixed} error{s}.")?;
} else {
writeln!(stdout, "Would fix {fixed} error{s}.")?;
writeln!(writer, "Would fix {fixed} error{s}.")?;
}
}
}
@ -155,7 +154,7 @@ impl Printer {
pub(crate) fn write_once(
&self,
diagnostics: &Diagnostics,
writer: &mut impl Write,
writer: &mut dyn Write,
) -> Result<()> {
if matches!(self.log_level, LogLevel::Silent) {
return Ok(());
@ -241,7 +240,11 @@ impl Printer {
Ok(())
}
pub(crate) fn write_statistics(&self, diagnostics: &Diagnostics) -> Result<()> {
pub(crate) fn write_statistics(
&self,
diagnostics: &Diagnostics,
writer: &mut dyn Write,
) -> Result<()> {
let statistics: Vec<ExpandedStatistics> = diagnostics
.messages
.iter()
@ -277,7 +280,6 @@ impl Printer {
return Ok(());
}
let mut stdout = BufWriter::new(io::stdout().lock());
match self.format {
SerializationFormat::Text => {
// Compute the maximum number of digits in the count and code, for all messages,
@ -302,7 +304,7 @@ impl Printer {
// By default, we mimic Flake8's `--statistics` format.
for statistic in statistics {
writeln!(
stdout,
writer,
"{:>count_width$}\t{:<code_width$}\t{}{}",
statistic.count.to_string().bold(),
statistic.code.to_string().red().bold(),
@ -321,7 +323,7 @@ impl Printer {
return Ok(());
}
SerializationFormat::Json => {
writeln!(stdout, "{}", serde_json::to_string_pretty(&statistics)?)?;
writeln!(writer, "{}", serde_json::to_string_pretty(&statistics)?)?;
}
_ => {
anyhow::bail!(
@ -331,12 +333,16 @@ impl Printer {
}
}
stdout.flush()?;
writer.flush()?;
Ok(())
}
pub(crate) fn write_continuously(&self, diagnostics: &Diagnostics) -> Result<()> {
pub(crate) fn write_continuously(
&self,
writer: &mut dyn Write,
diagnostics: &Diagnostics,
) -> Result<()> {
if matches!(self.log_level, LogLevel::Silent) {
return Ok(());
}
@ -353,19 +359,18 @@ impl Printer {
);
}
let mut stdout = BufWriter::new(io::stdout().lock());
if !diagnostics.messages.is_empty() {
if self.log_level >= LogLevel::Default {
writeln!(stdout)?;
writeln!(writer)?;
}
let context = EmitterContext::new(&diagnostics.source_kind);
TextEmitter::default()
.with_show_fix_status(show_fix_status(self.autofix_level))
.with_show_source(self.flags.contains(Flags::SHOW_SOURCE))
.emit(&mut stdout, &diagnostics.messages, &context)?;
.emit(writer, &diagnostics.messages, &context)?;
}
stdout.flush()?;
writer.flush()?;
Ok(())
}
@ -394,7 +399,7 @@ const fn show_fix_status(autofix_level: flags::FixMode) -> bool {
!autofix_level.is_apply()
}
fn print_fix_summary<T: Write>(stdout: &mut T, fixed: &FxHashMap<String, FixTable>) -> Result<()> {
fn print_fix_summary(writer: &mut dyn Write, fixed: &FxHashMap<String, FixTable>) -> Result<()> {
let total = fixed
.values()
.map(|table| table.values().sum::<usize>())
@ -410,14 +415,14 @@ fn print_fix_summary<T: Write>(stdout: &mut T, fixed: &FxHashMap<String, FixTabl
let s = if total == 1 { "" } else { "s" };
let label = format!("Fixed {total} error{s}:");
writeln!(stdout, "{}", label.bold().green())?;
writeln!(writer, "{}", label.bold().green())?;
for (filename, table) in fixed
.iter()
.sorted_by_key(|(filename, ..)| filename.as_str())
{
writeln!(
stdout,
writer,
"{} {}{}",
"-".cyan(),
relativize_path(filename).bold(),
@ -425,7 +430,7 @@ fn print_fix_summary<T: Write>(stdout: &mut T, fixed: &FxHashMap<String, FixTabl
)?;
for (rule, count) in table.iter().sorted_by_key(|(.., count)| Reverse(*count)) {
writeln!(
stdout,
writer,
" {count:>num_digits$} × {} ({})",
rule.noqa_code().to_string().red().bold(),
rule.as_ref(),

View file

@ -212,6 +212,8 @@ Options:
Ignore any `# noqa` comments
--format <FORMAT>
Output serialization format for violations [env: RUFF_FORMAT=] [possible values: text, json, json-lines, junit, grouped, github, gitlab, pylint, azure]
-o, --output-file <OUTPUT_FILE>
Specify file to write the linter output to (default: stdout)
--target-version <TARGET_VERSION>
The minimum Python version that should be supported [possible values: py37, py38, py39, py310, py311]
--config <CONFIG>