From 9b2da13ed4bf9035f8cf8487df6bf5be850a2fcb Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Sat, 21 Sep 2024 18:44:40 +0100 Subject: [PATCH] feat: Log errors as they are found Closes kemitix/forgejo-todo-checker#5 --- src/main.rs | 15 ++------- src/model/config.rs | 2 -- src/model/line.rs | 13 -------- src/model/markers.rs | 29 ++++++++++++++++++ src/model/markers/mod.rs | 62 -------------------------------------- src/model/markers/tests.rs | 55 --------------------------------- src/model/mod.rs | 1 - src/printer.rs | 1 + src/scanner.rs | 42 ++++++++++++++++++-------- src/tests/scanner.rs | 33 ++++++++++---------- 10 files changed, 77 insertions(+), 176 deletions(-) create mode 100644 src/model/markers.rs delete mode 100644 src/model/markers/mod.rs delete mode 100644 src/model/markers/tests.rs diff --git a/src/main.rs b/src/main.rs index fc3587c..6d3f51b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -29,20 +29,9 @@ async fn run(printer: &impl Printer, net: Network) -> Result<()> { let config = init_config(printer, net)?; let issues = fetch_open_issues(&config).await?; - let markers = find_markers(&config, issues, &DefaultFileScanner)?; + let errors = find_markers(printer, &config, issues, &DefaultFileScanner)?; - let mut errors = false; - for marker in (*markers).iter() { - match marker { - model::Marker::Closed(_, _) | model::Marker::Invalid(_) => { - printer.println("{marker}"); - errors = true; - } - model::Marker::Unmarked | model::Marker::Valid(_, _) => {} - } - } - - if errors { + if errors > 0 { bail!("Invalid or closed TODO/FIXMEs found") } Ok(()) diff --git a/src/model/config.rs b/src/model/config.rs index 6253436..84b72cb 100644 --- a/src/model/config.rs +++ b/src/model/config.rs @@ -1,6 +1,4 @@ // -#![allow(dead_code)] - use bon::Builder; use regex::Regex; diff --git a/src/model/line.rs b/src/model/line.rs index b1e01f8..fbf31e3 100644 --- a/src/model/line.rs +++ b/src/model/line.rs @@ -1,6 +1,4 @@ // -#![allow(dead_code)] - use std::path::PathBuf; use anyhow::{Context, Result}; @@ -15,22 +13,11 @@ use super::Marker; #[derive(Debug, Builder)] pub struct Line { - file: PathBuf, relative_path: PathBuf, num: usize, value: String, } impl Line { - // pub fn file(&self) -> &Path { - // &self.file - // } - // pub fn num(&self) -> usize { - // self.num - // } - // pub fn value(&self) -> &str { - // &self.value - // } - pub fn into_marker(self) -> Result { if marker_pattern()?.find(&self.value).is_some() { match issue_pattern()?.captures(&self.value) { diff --git a/src/model/markers.rs b/src/model/markers.rs new file mode 100644 index 0000000..1dee557 --- /dev/null +++ b/src/model/markers.rs @@ -0,0 +1,29 @@ +// +use crate::{issues::Issue, model::Line}; + +#[derive(Debug)] +pub enum Marker { + Unmarked, + Invalid(Line), + Valid(Line, Issue), + Closed(Line, Issue), +} +impl Marker { + pub fn into_closed(self) -> Self { + match self { + Self::Valid(line, issue) => Self::Closed(line, issue), + #[cfg(not(tarpaulin_include))] // only ever called when is a Valid + _ => self, + } + } +} + +impl std::fmt::Display for Marker { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Invalid(line) => write!(f, "- Invalid: {}", line), + Self::Closed(line, issue) => write!(f, "- Closed : ({issue}) {line}"), + Self::Valid(_, _) | Self::Unmarked => Ok(()), // tarpaulin uncovered okay + } + } +} diff --git a/src/model/markers/mod.rs b/src/model/markers/mod.rs deleted file mode 100644 index afb44ac..0000000 --- a/src/model/markers/mod.rs +++ /dev/null @@ -1,62 +0,0 @@ -// - -#[cfg(test)] -mod tests; - -use std::ops::Deref; - -use crate::{issues::Issue, model::Line}; - -#[derive(Debug)] -pub enum Marker { - Unmarked, - Invalid(Line), - #[allow(dead_code)] - Valid(Line, Issue), - Closed(Line, Issue), -} -impl Marker { - pub fn into_closed(self) -> Self { - match self { - Self::Valid(line, issue) => Self::Closed(line, issue), - #[cfg(not(tarpaulin_include))] // only ever called when is a Valid - _ => self, - } - } -} - -impl std::fmt::Display for Marker { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::Unmarked => Ok(()), - Self::Invalid(line) => write!(f, "- Invalid: {line}"), - Self::Valid(line, issue) => write!(f, "- Valid : ({issue}) {line}"), - Self::Closed(line, issue) => write!(f, "- Closed : ({issue}) {line}"), - } - } -} - -#[derive(Debug, Default)] -pub struct Markers { - markers: Vec, -} -impl Markers { - pub fn add_marker(&mut self, marker: Marker) { - self.markers.push(marker); - } -} -impl Deref for Markers { - type Target = Vec; - - fn deref(&self) -> &Self::Target { - &self.markers - } -} -impl std::fmt::Display for Markers { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - for marker in self.markers.iter() { - write!(f, "{marker}")?; - } - Ok(()) - } -} diff --git a/src/model/markers/tests.rs b/src/model/markers/tests.rs deleted file mode 100644 index 454986c..0000000 --- a/src/model/markers/tests.rs +++ /dev/null @@ -1,55 +0,0 @@ -// -use crate::model::{markers::Markers, Line}; - -#[test] -fn found_when_displayed() -> anyhow::Result<()> { - //given - let fs = kxio::fs::temp()?; - let file = fs.base().join("file-name"); - let relative = file.strip_prefix(fs.base())?.to_path_buf(); - - let mut found = Markers::default(); - - let marker_unmarked = Line::builder() - .file(file.clone()) - .relative_path(relative.clone()) - .num(10) - .value("line with no comment".to_owned()) - .build() - .into_marker()?; - let marker_invalid = Line::builder() - .file(file.clone()) - .relative_path(relative.clone()) - .num(10) - .value(format!("line // {}: comment", "TODO")) - .build() - .into_marker()?; - let marker_valid = Line::builder() - .file(file) - .relative_path(relative) - .num(11) - .value(format!("line // {}: (#13) do this", "TODO")) - .build() - .into_marker()?; - - found.add_marker(marker_unmarked); - found.add_marker(marker_invalid); - found.add_marker(marker_valid); - - //when - let markers_as_string = found.to_string(); - let result = markers_as_string.lines().collect::>(); - - //then - assert_eq!( - result, - vec![ - "- Invalid: file-name#10:", - format!(" line // {}: comment", "TODO").as_str(), - "- Valid : (13) file-name#11:", - format!(" line // {}: (#13) do this", "TODO").as_str() - ] - ); - - Ok(()) -} diff --git a/src/model/mod.rs b/src/model/mod.rs index ec08470..bddf6ca 100644 --- a/src/model/mod.rs +++ b/src/model/mod.rs @@ -9,4 +9,3 @@ mod tests; pub use config::Config; pub use line::Line; pub use markers::Marker; -pub use markers::Markers; diff --git a/src/printer.rs b/src/printer.rs index fce09b1..c3698a7 100644 --- a/src/printer.rs +++ b/src/printer.rs @@ -5,6 +5,7 @@ pub trait Printer { pub struct StandardPrinter; impl Printer for StandardPrinter { + #[cfg(not(tarpaulin_include))] fn println(&self, message: impl Into) { println!("{}", message.into()); } diff --git a/src/scanner.rs b/src/scanner.rs index 36ec1d7..b252ecb 100644 --- a/src/scanner.rs +++ b/src/scanner.rs @@ -3,36 +3,37 @@ use std::{collections::HashSet, path::Path}; use crate::{ issues::Issue, - model::{Config, Line, Marker, Markers}, + model::{Config, Line, Marker}, + printer::Printer, }; use anyhow::Result; use file_format::FileFormat; use ignore::Walk; -//<'a> = dyn Fn(&'a Path, &'a Config, &'a mut Markers, &'a HashSet, Output=Result<()>); pub trait FileScanner { fn scan_file( &self, path: &Path, config: &Config, - markers: &mut Markers, + printer: &impl Printer, issues: &HashSet, - ) -> Result<()>; + ) -> Result; } pub fn find_markers( + printer: &impl Printer, config: &Config, issues: HashSet, file_scanner: &impl FileScanner, -) -> Result { - let mut markers = Markers::default(); +) -> Result { + let mut errors = 0; for file in Walk::new(config.fs().base()).flatten() { let path = file.path(); if is_text_file(config, path)? { - file_scanner.scan_file(path, config, &mut markers, &issues)? + errors += file_scanner.scan_file(path, config, printer, &issues)? } } - Ok(markers) + Ok(errors) } fn is_text_file(config: &Config, path: &Path) -> Result { @@ -48,10 +49,11 @@ impl FileScanner for DefaultFileScanner { &self, file: &Path, config: &Config, - markers: &mut Markers, + printer: &impl Printer, issues: &HashSet, - ) -> Result<()> { + ) -> Result { let relative_path = file.strip_prefix(config.fs().base())?.to_path_buf(); + let mut errors = 0; config .fs() .file_read_to_string(file)? // tarpaulin uncovered okay @@ -59,7 +61,6 @@ impl FileScanner for DefaultFileScanner { .enumerate() .map(|(n, line)| { Line::builder() - .file(file.to_path_buf()) .relative_path(relative_path.clone()) .num(n + 1) // line numbers are not 0-based, but enumerate is .value(line.to_owned()) @@ -68,9 +69,24 @@ impl FileScanner for DefaultFileScanner { .filter_map(|line| line.into_marker().ok()) .filter(|marker| !matches!(marker, Marker::Unmarked)) .map(|marker| has_open_issue(marker, issues)) - .for_each(|marker| markers.add_marker(marker)); + .for_each(|marker| match marker { + Marker::Invalid(_) => { + errors += 1; + // TODO: (#6) Better error message + printer.println(marker.to_string()); + } + Marker::Closed(_, _) => { + errors += 1; + // TODO: (#6) Better error message + printer.println(marker.to_string()); + } + _ => {} + }); + if errors > 0 { + printer.println(format!(">> {errors} errors in {}", file.to_string_lossy())); + } - Ok(()) + Ok(errors) } } diff --git a/src/tests/scanner.rs b/src/tests/scanner.rs index 8e6ea2a..02b955d 100644 --- a/src/tests/scanner.rs +++ b/src/tests/scanner.rs @@ -9,13 +9,15 @@ use issues::Issue; use model::Config; use patterns::{issue_pattern, marker_pattern}; use pretty_assertions::assert_eq; +use printer::TestPrinter; #[test] fn find_markers_in_dir() -> anyhow::Result<()> { //given let fs = kxio::fs::temp()?; + let file_with_invalids = fs.base().join("file_with_invalids.txt"); fs.file_write( - &fs.base().join("file_with_invalids.txt"), + &file_with_invalids, include_str!("data/file_with_invalids.txt"), )?; fs.file_write( @@ -32,26 +34,22 @@ fn find_markers_in_dir() -> anyhow::Result<()> { .issue_pattern(issue_pattern()?) .build(); let issues = HashSet::from_iter(vec![Issue::new(23), Issue::new(43)]); + let printer = TestPrinter::default(); //when - let markers = find_markers(&config, issues, &DefaultFileScanner)?; + let errors = find_markers(&printer, &config, issues, &DefaultFileScanner)?; //then assert_eq!( - markers.to_string().lines().collect::>(), + printer.messages.take(), vec![ - "- Invalid: file_with_invalids.txt#3:", - " It contains a todo comment: // TODO: this is it", - "- Invalid: file_with_invalids.txt#5:", - " It also contains a fix-me comment: // FIXME: and this is it", - "- Closed : (3) file_with_invalids.txt#9:", - " We also have a todo comment: // TODO: (#3) and it has an issue number, but it is closed", - "- Valid : (23) file_with_valids.txt#3:", - " It also has a todo comment: // TODO: (#23) and it has an issue number", - "- Valid : (43) file_with_valids.txt#5:", - " Here is a fix-me comment: // FIXME: (#43) and is also has an issue number" + "- Invalid: file_with_invalids.txt#3:\n It contains a todo comment: // TODO: this is it\n", + "- Invalid: file_with_invalids.txt#5:\n It also contains a fix-me comment: // FIXME: and this is it\n", + "- Closed : (3) file_with_invalids.txt#9:\n We also have a todo comment: // TODO: (#3) and it has an issue number, but it is closed\n", + format!(">> 3 errors in {}", file_with_invalids.to_string_lossy()).as_str() ] ); + assert_eq!(errors, 3); Ok(()) } @@ -70,9 +68,10 @@ fn skips_binary_files() -> Result<()> { let config = a_config(net, fs)?; let issues = HashSet::new(); let file_scanner = TestFileScanner::default(); + let printer = TestPrinter::default(); //when - find_markers(&config, issues, &file_scanner)?; + find_markers(&printer, &config, issues, &file_scanner)?; //then assert_eq!(file_scanner.scanned.take(), vec![text_path]); @@ -89,10 +88,10 @@ impl FileScanner for TestFileScanner { &self, path: &std::path::Path, _config: &Config, - _markers: &mut model::Markers, + _printer: &impl Printer, _issues: &HashSet, - ) -> Result<()> { + ) -> Result { self.scanned.borrow_mut().push(path.to_path_buf()); - Ok(()) + Ok(0) } }