From 609440806766812852fd7bdc6d615b5a770dbfeb Mon Sep 17 00:00:00 2001 From: gvozdvmozgu Date: Fri, 1 Mar 2024 17:54:03 -0500 Subject: [PATCH] chore: refactor linting CLI to use direct paths and improve output formatting --- Cargo.lock | 1 - crates/cli/Cargo.toml | 1 - crates/cli/src/commands.rs | 5 +- crates/cli/src/main.rs | 71 ++++--------------- crates/cli/tests/ui/LT01_LT012.stderr | 3 +- .../test_fail_whitespace_before_comma.stderr | 3 +- crates/lib/src/cli/formatters.rs | 9 ++- crates/lib/src/core/linter.rs | 1 + crates/lib/src/core/linter/linter.rs | 46 ++++++++++-- crates/lib/src/core/linter/runner.rs | 33 +++++++++ 10 files changed, 99 insertions(+), 74 deletions(-) create mode 100644 crates/lib/src/core/linter/runner.rs diff --git a/Cargo.lock b/Cargo.lock index 8750c42df..a98a4ac65 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -929,7 +929,6 @@ name = "sqruff" version = "0.1.4" dependencies = [ "clap", - "glob", "sqruff-lib", "tikv-jemallocator", "ui_test", diff --git a/crates/cli/Cargo.toml b/crates/cli/Cargo.toml index e9ee8bf06..0e6351885 100644 --- a/crates/cli/Cargo.toml +++ b/crates/cli/Cargo.toml @@ -15,7 +15,6 @@ jemalloc = ["jemallocator"] [dependencies] sqruff-lib = { version = "0.1.4", path = "../lib" } clap = { version = "4", features = ["derive"] } -glob = "0.3.1" [target.'cfg(not(target_env = "msvc"))'.dependencies] jemallocator = { version = "0.5", package = "tikv-jemallocator", optional = true } diff --git a/crates/cli/src/commands.rs b/crates/cli/src/commands.rs index b8af9c605..89c2fe6db 100644 --- a/crates/cli/src/commands.rs +++ b/crates/cli/src/commands.rs @@ -1,3 +1,5 @@ +use std::path::PathBuf; + use clap::{Parser, Subcommand}; #[derive(Debug, Parser)] @@ -18,8 +20,7 @@ pub enum Commands { #[derive(Debug, Parser)] pub struct LintArgs { - /// glob pattern to lint - pub file_path: String, + pub paths: Vec, } #[derive(Debug, Parser)] diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 646e07edb..83bc0b8e2 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -1,13 +1,10 @@ -use std::process::exit; - -use clap::Parser; -use glob::{glob, Paths}; -use sqruff_lib::api::simple::lint_with_formatter; +use clap::Parser as _; +use commands::LintArgs; use sqruff_lib::cli::formatters::OutputStreamFormatter; use sqruff_lib::core::config::FluffConfig; -use sqruff_lib::rules::layout; +use sqruff_lib::core::linter::linter::Linter; -use crate::commands::Cli; +use crate::commands::{Cli, Commands}; mod commands; @@ -16,62 +13,18 @@ mod commands; static ALLOC: jemallocator::Jemalloc = jemallocator::Jemalloc; fn main() { - match main_wrapper() { - Ok(msg) => { - println!("{}", msg); - exit(0) - } - Err(e) => { - eprintln!("{}", e); - exit(1) - } - } -} + let formatter = OutputStreamFormatter::new(Box::new(std::io::stderr()), false); + let config = FluffConfig::default(); -// TODO Handle the unwraps better -fn main_wrapper() -> Result { let cli = Cli::parse(); - let mut has_errors = false; - - let config = FluffConfig::from_root(None, false, None).unwrap(); + let mut linter = Linter::new(config, formatter.into(), None); match cli.command { - commands::Commands::Lint(lint_args) => { - let files = find_files(&lint_args.file_path)?; - - let mut count = 0; - for file in files { - let formatter = OutputStreamFormatter::new(Box::new(std::io::stderr()), false); - - let file = file.unwrap(); - let file = file.to_str().unwrap(); - let contents = std::fs::read_to_string(file).unwrap(); - let linted = lint_with_formatter( - contents, - // TODO Make this a pointer - DEFAULT_DIALECT.to_string(), - layout::get_rules().into(), - None, - None, - formatter.into(), - ) - .map_err(|e| format!("Error linting file '{}': {:?}", file, e))?; - if !linted.is_empty() { - has_errors = true; - } - count += 1; - } - } - commands::Commands::Fix(_) => { - unimplemented!(); + Commands::Lint(LintArgs { paths }) => { + linter.lint_paths(paths); } - }; - - if !has_errors { Ok(String::new()) } else { Err(String::new()) } -} - -const DEFAULT_DIALECT: &str = "ansi"; + Commands::Fix(_) => todo!(), + } -fn find_files(pattern: &str) -> Result { - glob(pattern).map_err(|e| format!("Error finding files with pattern '{}': {:?}", pattern, e)) + std::process::exit(if linter.formatter.unwrap().has_fail.get() { 1 } else { 0 }) } diff --git a/crates/cli/tests/ui/LT01_LT012.stderr b/crates/cli/tests/ui/LT01_LT012.stderr index 429301510..1ebbb60dc 100644 --- a/crates/cli/tests/ui/LT01_LT012.stderr +++ b/crates/cli/tests/ui/LT01_LT012.stderr @@ -1,4 +1,3 @@ -== [] FAIL +== [tests/ui/LT01_LT012.sql] FAIL L: 1 | P: 7 | LT01 | Expected only single space before "1". Found " ". [layout.spacing] L: 1 | P: 11 | LT12 | Files must end with a single trailing newline. [layout.end_of_file] - diff --git a/crates/cli/tests/ui/test_fail_whitespace_before_comma.stderr b/crates/cli/tests/ui/test_fail_whitespace_before_comma.stderr index 07a278f75..a2ba5da54 100644 --- a/crates/cli/tests/ui/test_fail_whitespace_before_comma.stderr +++ b/crates/cli/tests/ui/test_fail_whitespace_before_comma.stderr @@ -1,7 +1,6 @@ -== [] FAIL +== [tests/ui/test_fail_whitespace_before_comma.sql] FAIL L: 1 | P: 1 | LT09 | Select targets should be on a new line unless there is only one select target. | [layout.select_targets] L: 1 | P: 9 | LT01 | Unexpected whitespace before ",". [layout.spacing] L: 1 | P: 11 | LT01 | Expected single whitespace between "," and "4". [layout.spacing] L: 1 | P: 11 | LT12 | Files must end with a single trailing newline. [layout.end_of_file] - diff --git a/crates/lib/src/cli/formatters.rs b/crates/lib/src/cli/formatters.rs index d94bba7ca..c14ceee1b 100644 --- a/crates/lib/src/cli/formatters.rs +++ b/crates/lib/src/cli/formatters.rs @@ -1,4 +1,5 @@ use std::borrow::Cow; +use std::cell::Cell; use std::io::{IsTerminal, Write}; use anstyle::{AnsiColor, Effects, Style}; @@ -56,6 +57,7 @@ pub struct OutputStreamFormatter { filter_empty: bool, verbosity: i32, output_line_length: usize, + pub has_fail: Cell, } impl OutputStreamFormatter { @@ -66,6 +68,7 @@ impl OutputStreamFormatter { filter_empty: true, verbosity: 0, output_line_length: 80, + has_fail: false.into(), } } @@ -149,6 +152,7 @@ impl OutputStreamFormatter { fname, linted_file.get_violations(only_fixable.then_some(true)), ); + self.dispatch(&s); } @@ -169,7 +173,10 @@ impl OutputStreamFormatter { let color = match status { Status::Pass | Status::Fixed => AnsiColor::Green, - Status::Fail | Status::Error => AnsiColor::Red, + Status::Fail | Status::Error => { + self.has_fail.set(true); + AnsiColor::Red + } } .on_default(); diff --git a/crates/lib/src/core/linter.rs b/crates/lib/src/core/linter.rs index dacdbe311..ffdfb447b 100644 --- a/crates/lib/src/core/linter.rs +++ b/crates/lib/src/core/linter.rs @@ -3,3 +3,4 @@ pub mod linted_dir; pub mod linted_file; pub mod linter; pub mod linting_result; +mod runner; diff --git a/crates/lib/src/core/linter/linter.rs b/crates/lib/src/core/linter/linter.rs index 220ec2cda..8e7e7a444 100644 --- a/crates/lib/src/core/linter/linter.rs +++ b/crates/lib/src/core/linter/linter.rs @@ -1,7 +1,7 @@ use std::collections::{HashMap, HashSet}; use std::fs::File; use std::io::{BufRead, BufReader}; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::time::Instant; use itertools::Itertools; @@ -10,6 +10,7 @@ use uuid::Uuid; use walkdir::WalkDir; use super::linted_dir::LintedDir; +use super::runner::RunnerContext; use crate::cli::formatters::OutputStreamFormatter; use crate::core::config::FluffConfig; use crate::core::dialects::init::dialect_selector; @@ -26,8 +27,9 @@ use crate::core::templaters::base::{RawTemplater, TemplatedFile, Templater}; pub struct Linter { config: FluffConfig, - pub(crate) formatter: Option, + pub formatter: Option, templater: Box, + rules: Vec, } impl Linter { @@ -37,8 +39,15 @@ impl Linter { templater: Option>, ) -> Linter { match templater { - Some(templater) => Linter { config, formatter, templater }, - None => Linter { config, formatter, templater: Box::::default() }, + Some(templater) => { + Linter { config, formatter, templater, rules: crate::rules::layout::get_rules() } + } + None => Linter { + config, + formatter, + templater: Box::::default(), + rules: crate::rules::layout::get_rules(), + }, } } @@ -143,6 +152,31 @@ impl Linter { self.lint_parsed(parsed, rules, fix) } + pub fn lint_paths(&mut self, mut paths: Vec) { + if paths.is_empty() { + paths.push(std::env::current_dir().unwrap()); + } + + let mut expanded_paths = Vec::new(); + for path in paths { + let paths = self.paths_from_path(path, None, None, None, None); + expanded_paths.extend(paths); + } + + let mut runner = RunnerContext::sequential(self); + runner.run(expanded_paths); + } + + pub fn render_file(&mut self, fname: String) -> RenderedFile { + let in_str = std::fs::read_to_string(&fname).unwrap(); + self.render_string(in_str, fname, self.config.clone(), None).unwrap() + } + + pub fn lint_rendered(&mut self, rendered: RenderedFile) { + let parsed = Self::parse_rendered(rendered, false); + self.lint_parsed(parsed, self.rules.clone(), false); + } + pub fn lint_parsed( &mut self, parsed_string: ParsedString, @@ -326,7 +360,7 @@ impl Linter { config, time_dict: HashMap::new(), f_name: f_name.to_owned(), - encoding: encoding.to_owned().unwrap(), + encoding: encoding.to_owned().unwrap_or_else(|| "UTF-8".into()), source_str: f_name.to_owned(), }) } @@ -457,7 +491,7 @@ impl Linter { // look for an ignore file in the direct parent of the file. fn paths_from_path( &self, - path: String, + path: PathBuf, ignore_file_name: Option, ignore_non_existent_files: Option, ignore_files: Option, diff --git a/crates/lib/src/core/linter/runner.rs b/crates/lib/src/core/linter/runner.rs new file mode 100644 index 000000000..fd2b409c9 --- /dev/null +++ b/crates/lib/src/core/linter/runner.rs @@ -0,0 +1,33 @@ +use super::linter::Linter; + +pub trait Runner: Sized { + fn run(&mut self, paths: Vec, linter: &mut Linter); +} + +pub struct RunnerContext<'me, R> { + linter: &'me mut Linter, + runner: R, +} + +impl<'me> RunnerContext<'me, SequentialRunner> { + pub fn sequential(linter: &'me mut Linter) -> Self { + Self { linter, runner: SequentialRunner } + } +} + +impl RunnerContext<'_, R> { + pub fn run(&mut self, paths: Vec) { + self.runner.run(paths, &mut self.linter); + } +} + +pub struct SequentialRunner; + +impl Runner for SequentialRunner { + fn run(&mut self, paths: Vec, linter: &mut Linter) { + for path in paths { + let rendered = linter.render_file(path); + linter.lint_rendered(rendered); + } + } +}