Skip to content

Commit

Permalink
refactor(linter): move finishing default diagnostic message to `Graph…
Browse files Browse the repository at this point in the history
…icalReporter` (#8683)

Now every lint output is owned by is right OutputFormatter and his DiagnosticReporter 🥳
Next step is to setup a snapshot Tester, so I can remove the ToDos.

Reorded some lines so the outfor is now for: `cargo run -p oxlint -- test.js --max-warnings=2`
```
Found 4 warnings and 0 errors.
Exceeded maximum number of warnings. Found 4.
Finished in 5ms on 1 file with 97 rules using 24 threads.
```

and for `cargo run -p oxlint -- test.js`

```
Found 4 warnings and 0 errors.
Finished in 5ms on 1 file with 97 rules using 24 threads.
```

The output time and warnings/error count wil be always printed.
  • Loading branch information
Sysix committed Jan 24, 2025
1 parent b977678 commit 10e5920
Show file tree
Hide file tree
Showing 7 changed files with 189 additions and 94 deletions.
44 changes: 28 additions & 16 deletions apps/oxlint/src/lint.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use std::{
env, fs,
io::{BufWriter, Write},
io::{BufWriter, ErrorKind, Write},
path::{Path, PathBuf},
process::ExitCode,
time::Instant,
};

Expand All @@ -16,7 +17,7 @@ use serde_json::Value;

use crate::{
cli::{CliRunResult, LintCommand, LintResult, MiscOptions, Runner, WarningOptions},
output_formatter::{OutputFormat, OutputFormatter},
output_formatter::{LintCommandInfo, OutputFormatter},
walk::{Extensions, Walk},
};

Expand Down Expand Up @@ -55,7 +56,6 @@ impl Runner for LintRunner {
ignore_options,
fix_options,
enable_plugins,
output_options,
misc_options,
..
} = self.options;
Expand All @@ -81,11 +81,8 @@ impl Runner for LintRunner {
// If explicit paths were provided, but all have been
// filtered, return early.
if provided_path_count > 0 {
return CliRunResult::LintResult(LintResult {
duration: now.elapsed(),
deny_warnings: warning_options.deny_warnings,
..LintResult::default()
});
// ToDo: when oxc_linter (config) validates the configuration, we can use exit_code = 1 to fail
return CliRunResult::LintResult(LintResult::default());
}

paths.push(self.cwd.clone());
Expand Down Expand Up @@ -211,15 +208,24 @@ impl Runner for LintRunner {

let diagnostic_result = diagnostic_service.run(&mut stdout);

CliRunResult::LintResult(LintResult {
duration: now.elapsed(),
let diagnostic_failed = diagnostic_result.max_warnings_exceeded()
|| diagnostic_result.errors_count() > 0
|| (warning_options.deny_warnings && diagnostic_result.warnings_count() > 0);

if let Some(end) = output_formatter.lint_command_info(&LintCommandInfo {
number_of_files,
number_of_rules: lint_service.linter().number_of_rules(),
threads_count: rayon::current_num_threads(),
start_time: now.elapsed(),
}) {
stdout.write_all(end.as_bytes()).or_else(Self::check_for_writer_error).unwrap();
};

CliRunResult::LintResult(LintResult {
number_of_files,
number_of_warnings: diagnostic_result.warnings_count(),
number_of_errors: diagnostic_result.errors_count(),
max_warnings_exceeded: diagnostic_result.max_warnings_exceeded(),
deny_warnings: warning_options.deny_warnings,
print_summary: matches!(output_options.format, OutputFormat::Default),
exit_code: ExitCode::from(u8::from(diagnostic_failed)),
})
}
}
Expand Down Expand Up @@ -306,6 +312,15 @@ impl LintRunner {
let config_path = cwd.join(Self::DEFAULT_OXLINTRC);
Oxlintrc::from_file(&config_path).or_else(|_| Ok(Oxlintrc::default()))
}

fn check_for_writer_error(error: std::io::Error) -> Result<(), std::io::Error> {
// Do not panic when the process is killed (e.g. piping into `less`).
if matches!(error.kind(), ErrorKind::Interrupted | ErrorKind::BrokenPipe) {
Ok(())
} else {
Err(error)
}
}
}

#[cfg(test)]
Expand Down Expand Up @@ -364,7 +379,6 @@ mod test {
fn no_arg() {
let args = &[];
let result = test(args);
assert!(result.number_of_rules > 0);
assert!(result.number_of_warnings > 0);
assert_eq!(result.number_of_errors, 0);
}
Expand All @@ -373,7 +387,6 @@ mod test {
fn dir() {
let args = &["fixtures/linter"];
let result = test(args);
assert!(result.number_of_rules > 0);
assert_eq!(result.number_of_files, 3);
assert_eq!(result.number_of_warnings, 3);
assert_eq!(result.number_of_errors, 0);
Expand All @@ -383,7 +396,6 @@ mod test {
fn cwd() {
let args = &["debugger.js"];
let result = test_with_cwd("fixtures/linter", args);
assert!(result.number_of_rules > 0);
assert_eq!(result.number_of_files, 1);
assert_eq!(result.number_of_warnings, 1);
assert_eq!(result.number_of_errors, 0);
Expand Down
4 changes: 2 additions & 2 deletions apps/oxlint/src/output_formatter/checkstyle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ fn format_checkstyle(diagnostics: &[Error]) -> String {
format!(r#"<file name="{filename}">{messages}</file>"#)
}).collect::<Vec<_>>().join(" ");
format!(
r#"<?xml version="1.0" encoding="utf-8"?><checkstyle version="4.3">{messages}</checkstyle>"#
"<?xml version=\"1.0\" encoding=\"utf-8\"?><checkstyle version=\"4.3\">{messages}</checkstyle>\n"
)
}

Expand Down Expand Up @@ -148,6 +148,6 @@ mod test {
let second_result = reporter.finish(&DiagnosticResult::default());

assert!(second_result.is_some());
assert_eq!(second_result.unwrap(), "<?xml version=\"1.0\" encoding=\"utf-8\"?><checkstyle version=\"4.3\"><file name=\"file://test.ts\"><error line=\"1\" column=\"1\" severity=\"warning\" message=\"error message\" source=\"\" /></file></checkstyle>");
assert_eq!(second_result.unwrap(), "<?xml version=\"1.0\" encoding=\"utf-8\"?><checkstyle version=\"4.3\"><file name=\"file://test.ts\"><error line=\"1\" column=\"1\" severity=\"warning\" message=\"error message\" source=\"\" /></file></checkstyle>\n");
}
}
115 changes: 109 additions & 6 deletions apps/oxlint/src/output_formatter/default.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::io::Write;
use std::{io::Write, time::Duration};

use oxc_diagnostics::{
reporter::{DiagnosticReporter, DiagnosticResult},
Expand All @@ -21,11 +21,34 @@ impl InternalFormatter for DefaultOutputFormatter {
writeln!(writer, "Total: {}", table.total).unwrap();
}

fn lint_command_info(&self, lint_command_info: &super::LintCommandInfo) -> Option<String> {
let time = Self::get_execution_time(&lint_command_info.start_time);
let s = if lint_command_info.number_of_files == 1 { "" } else { "s" };

Some(format!(
"Finished in {time} on {} file{s} with {} rules using {} threads.\n",
lint_command_info.number_of_files,
lint_command_info.number_of_rules,
lint_command_info.threads_count
))
}

fn get_diagnostic_reporter(&self) -> Box<dyn DiagnosticReporter> {
Box::new(GraphicalReporter::default())
}
}

impl DefaultOutputFormatter {
fn get_execution_time(duration: &Duration) -> String {
let ms = duration.as_millis();
if ms < 1000 {
format!("{ms}ms")
} else {
format!("{:.1}s", duration.as_secs_f64())
}
}
}

/// Pretty-prints diagnostics. Primarily meant for human-readable output in a terminal.
///
/// See [`GraphicalReportHandler`] for how to configure colors, context lines, etc.
Expand All @@ -40,8 +63,35 @@ impl Default for GraphicalReporter {
}

impl DiagnosticReporter for GraphicalReporter {
fn finish(&mut self, _: &DiagnosticResult) -> Option<String> {
None
fn finish(&mut self, result: &DiagnosticResult) -> Option<String> {
let mut output = String::new();

if result.warnings_count() + result.errors_count() > 0 {
output.push('\n');
}

output.push_str(
format!(
"Found {} warning{} and {} error{}.\n",
result.warnings_count(),
if result.warnings_count() == 1 { "" } else { "s" },
result.errors_count(),
if result.errors_count() == 1 { "" } else { "s" },
)
.as_str(),
);

if result.max_warnings_exceeded() {
output.push_str(
format!(
"Exceeded maximum number of warnings. Found {}.\n",
result.warnings_count()
)
.as_str(),
);
}

Some(output)
}

fn render_error(&mut self, error: Error) -> Option<String> {
Expand All @@ -60,9 +110,11 @@ impl GraphicalReporter {

#[cfg(test)]
mod test {
use std::time::Duration;

use crate::output_formatter::{
default::{DefaultOutputFormatter, GraphicalReporter},
InternalFormatter,
InternalFormatter, LintCommandInfo,
};
use miette::{GraphicalReportHandler, GraphicalTheme, NamedSource};
use oxc_diagnostics::{
Expand All @@ -81,12 +133,63 @@ mod test {
}

#[test]
fn reporter_finish() {
fn lint_command_info() {
let formatter = DefaultOutputFormatter;
let result = formatter.lint_command_info(&LintCommandInfo {
number_of_files: 5,
number_of_rules: 10,
threads_count: 12,
start_time: Duration::new(1, 0),
});

assert!(result.is_some());
assert_eq!(
result.unwrap(),
"Finished in 1.0s on 5 files with 10 rules using 12 threads.\n"
);
}

#[test]
fn reporter_finish_no_results() {
let mut reporter = GraphicalReporter::default();

let result = reporter.finish(&DiagnosticResult::default());

assert!(result.is_none());
assert!(result.is_some());
assert_eq!(result.unwrap(), "Found 0 warnings and 0 errors.\n");
}

#[test]
fn reporter_finish_one_warning_and_one_error() {
let mut reporter = GraphicalReporter::default();

let result = reporter.finish(&DiagnosticResult::new(1, 1, false));

assert!(result.is_some());
assert_eq!(result.unwrap(), "\nFound 1 warning and 1 error.\n");
}

#[test]
fn reporter_finish_multiple_warning_and_errors() {
let mut reporter = GraphicalReporter::default();

let result = reporter.finish(&DiagnosticResult::new(6, 4, false));

assert!(result.is_some());
assert_eq!(result.unwrap(), "\nFound 6 warnings and 4 errors.\n");
}

#[test]
fn reporter_finish_exceeded_warnings() {
let mut reporter = GraphicalReporter::default();

let result = reporter.finish(&DiagnosticResult::new(6, 4, true));

assert!(result.is_some());
assert_eq!(
result.unwrap(),
"\nFound 6 warnings and 4 errors.\nExceeded maximum number of warnings. Found 6.\n"
);
}

#[test]
Expand Down
4 changes: 2 additions & 2 deletions apps/oxlint/src/output_formatter/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ fn format_json(diagnostics: &mut Vec<Error>) -> String {
})
.collect::<Vec<_>>()
.join(",\n");
format!("[\n{messages}\n]")
format!("[\n{messages}\n]\n")
}

#[cfg(test)]
Expand Down Expand Up @@ -108,7 +108,7 @@ mod test {
assert!(second_result.is_some());
assert_eq!(
second_result.unwrap(),
"[\n\t{\"message\": \"error message\",\"severity\": \"warning\",\"causes\": [],\"filename\": \"file://test.ts\",\"labels\": [{\"span\": {\"offset\": 0,\"length\": 8}}],\"related\": []}\n]"
"[\n\t{\"message\": \"error message\",\"severity\": \"warning\",\"causes\": [],\"filename\": \"file://test.ts\",\"labels\": [{\"span\": {\"offset\": 0,\"length\": 8}}],\"related\": []}\n]\n"
);
}
}
44 changes: 40 additions & 4 deletions apps/oxlint/src/output_formatter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ mod unix;

use std::io::{BufWriter, Stdout, Write};
use std::str::FromStr;
use std::time::Duration;

use checkstyle::CheckStyleOutputFormatter;
use github::GithubOutputFormatter;
Expand Down Expand Up @@ -45,19 +46,45 @@ impl FromStr for OutputFormat {
}
}

/// Some extra lint information, which can be outputted
/// at the end of the command
pub struct LintCommandInfo {
/// The number of files that were linted.
pub number_of_files: usize,
/// The number of lint rules that were run.
pub number_of_rules: usize,
/// The used CPU threads count
pub threads_count: usize,
/// Some reporters want to output the duration it took to finished the task
pub start_time: Duration,
}

/// An Interface for the different output formats.
/// The Formatter is then managed by [`OutputFormatter`].
trait InternalFormatter {
/// Print all available rules by oxlint
/// Some Formatter do not know how to output the rules in the style,
/// instead you should print out that this combination of flags is not supported.
/// Example: "flag --rules with flag --format=checkstyle is not allowed"
fn all_rules(&mut self, writer: &mut dyn Write);

/// At the end of the Lint command the Formatter can output extra information.
fn lint_command_info(&self, _lint_command_info: &LintCommandInfo) -> Option<String> {
None
}

/// oxlint words with [`DiagnosticService`](oxc_diagnostics::DiagnosticService),
/// which uses a own reporter to output to stdout.
fn get_diagnostic_reporter(&self) -> Box<dyn DiagnosticReporter>;
}

pub struct OutputFormatter {
internal_formatter: Box<dyn InternalFormatter>,
internal: Box<dyn InternalFormatter>,
}

impl OutputFormatter {
pub fn new(format: OutputFormat) -> Self {
Self { internal_formatter: Self::get_internal_formatter(format) }
Self { internal: Self::get_internal_formatter(format) }
}

fn get_internal_formatter(format: OutputFormat) -> Box<dyn InternalFormatter> {
Expand All @@ -71,11 +98,20 @@ impl OutputFormatter {
}
}

/// Print all available rules by oxlint
/// See [`InternalFormatter::all_rules`] for more details.
pub fn all_rules(&mut self, writer: &mut BufWriter<Stdout>) {
self.internal_formatter.all_rules(writer);
self.internal.all_rules(writer);
}

/// At the end of the Lint command we may output extra information.
pub fn lint_command_info(&self, lint_command_info: &LintCommandInfo) -> Option<String> {
self.internal.lint_command_info(lint_command_info)
}

/// Returns the [`DiagnosticReporter`] which then will be used by [`DiagnosticService`](oxc_diagnostics::DiagnosticService)
/// See [`InternalFormatter::get_diagnostic_reporter`] for more details.
pub fn get_diagnostic_reporter(&self) -> Box<dyn DiagnosticReporter> {
self.internal_formatter.get_diagnostic_reporter()
self.internal.get_diagnostic_reporter()
}
}
Loading

0 comments on commit 10e5920

Please sign in to comment.