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

Add "no-curly-quotes" rule to the codebase. #56

Merged
merged 12 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .trunk/config/toolbox.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,6 @@ enabled = false
[neveredit]
enabled = false
paths = []

[nocurlyquotes]
enabled = false
9 changes: 9 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ pub struct Conf {

#[config(nested)]
pub neveredit: NeverEditConf,

#[config(nested)]
pub nocurlyquotes: NoCurlyQuotesConf,
}

impl Conf {
Expand Down Expand Up @@ -49,3 +52,9 @@ pub struct NeverEditConf {
#[config(default = [])]
pub paths: Vec<String>,
}

#[derive(Config)]
pub struct NoCurlyQuotesConf {
#[config(default = false)]
pub enabled: bool,
}
61 changes: 34 additions & 27 deletions src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,7 @@ impl Diagnostic {
}

let fixes = if let Some(replacements) = &self.replacements {
let mut fixes = Vec::new();
for replacement in replacements {
fixes.push(replacement.to_fix(self));
}
Some(fixes)
Some(vec![self.build_fix(replacements)])
} else {
None
};
Expand All @@ -109,36 +105,47 @@ impl Diagnostic {
.build()
.unwrap()
}
}

impl Replacement {
pub fn to_fix(&self, diag: &Diagnostic) -> sarif::Fix {
pub fn build_fix(&self, replacements: &[Replacement]) -> sarif::Fix {
sarif::FixBuilder::default()
.artifact_changes([sarif::ArtifactChangeBuilder::default()
.artifact_location(
sarif::ArtifactLocationBuilder::default()
.uri(diag.path.clone())
.uri(self.path.clone())
.build()
.unwrap(),
)
.replacements(vec![sarif::ReplacementBuilder::default()
.deleted_region(
sarif::RegionBuilder::default()
.start_line(self.deleted_region.start.line as i64)
.start_column(self.deleted_region.start.character as i64 + 1)
.end_line(self.deleted_region.end.line as i64)
.end_column(self.deleted_region.end.character as i64 + 1)
.build()
.unwrap(),
)
.inserted_content(
sarif::ArtifactContentBuilder::default()
.text(self.inserted_content.clone())
.build()
.unwrap(),
)
.build()
.unwrap()])
.replacements(
replacements
.iter()
.map(|replacement| {
sarif::ReplacementBuilder::default()
.deleted_region(
sarif::RegionBuilder::default()
.start_line(
replacement.deleted_region.start.line as i64 + 1,
)
.start_column(
replacement.deleted_region.start.character as i64 + 1,
)
.end_line(replacement.deleted_region.end.line as i64 + 1)
.end_column(
replacement.deleted_region.end.character as i64 + 1,
)
.build()
.unwrap(),
)
.inserted_content(
sarif::ArtifactContentBuilder::default()
.text(replacement.inserted_content.clone())
.build()
.unwrap(),
)
.build()
.unwrap()
})
.collect::<Vec<_>>(),
)
.build()
.unwrap()])
.build()
Expand Down
7 changes: 7 additions & 0 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use horton::config::Conf;
use horton::diagnostic;
use horton::rules::if_change_then_change::ictc;
use horton::rules::never_edit::never_edit;
use horton::rules::no_curly_quotes::no_curly_quotes;
use horton::rules::pls_no_land::pls_no_land;
use horton::run::{Cli, OutputFormat, Run, Subcommands};

Expand Down Expand Up @@ -164,6 +165,12 @@ fn run() -> anyhow::Result<()> {
Err(e) => return Err(e),
}

let ncq_result = no_curly_quotes(&run, &cli.upstream);
match ncq_result {
Ok(result) => ret.diagnostics.extend(result),
Err(e) => return Err(e),
}

let mut output_string = generate_line_string(&ret);
if cli.output_format == OutputFormat::Sarif {
output_string = generate_sarif_string(&ret, &run, &start)?;
Expand Down
1 change: 1 addition & 0 deletions src/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
pub mod if_change_then_change;
pub mod never_edit;
pub mod no_curly_quotes;
pub mod pls_no_land;
106 changes: 106 additions & 0 deletions src/rules/no_curly_quotes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
use std::fs::File;
use std::io::BufRead;
use std::io::BufReader;
use std::path::PathBuf;

use crate::run::Run;

use anyhow::Context;
use log::debug;
use log::trace;
use rayon::iter::{IntoParallelRefIterator, ParallelIterator};

use crate::diagnostic::{Diagnostic, Position, Range, Replacement, Severity};

pub fn no_curly_quotes(run: &Run, _upstream: &str) -> anyhow::Result<Vec<Diagnostic>> {
let config = &run.config.nocurlyquotes;

if !config.enabled {
trace!("'nocurlyquotes' is disabled");
return Ok(vec![]);
}

debug!("scanning {} files for curly quotes", run.paths.len());

// Scan files in parallel
let results: Result<Vec<_>, _> = run.paths.par_iter().map(no_curly_quotes_impl).collect();

match results {
Ok(v) => Ok(v.into_iter().flatten().collect()),
Err(e) => Err(e),
}
}

const DOUBLE_CURLY_QUOTES: [char; 4] = ['\u{201C}', '\u{201D}', '\u{201E}', '\u{201F}'];
const SINGLE_CURLY_QUOTES: [char; 2] = ['\u{2018}', '\u{2019}'];

fn no_curly_quotes_impl(path: &PathBuf) -> anyhow::Result<Vec<Diagnostic>> {
let in_file = File::open(path).with_context(|| format!("failed to open: {:#?}", path))?;
let in_buf = BufReader::new(in_file);

trace!("scanning contents of {}", path.display());

let lines_view = in_buf
.lines()
.collect::<std::io::Result<Vec<String>>>()
.with_context(|| format!("failed to read lines of text from {:#?}", path))?;

let mut ret = Vec::new();

for (i, line) in lines_view.iter().enumerate() {
let mut char_issues = Vec::new();

for (pos, c) in line.char_indices() {
if SINGLE_CURLY_QUOTES.contains(&c) {
let char_pos = line[..pos].chars().count() as u64;
char_issues.push((char_pos, "'"));
}
if DOUBLE_CURLY_QUOTES.contains(&c) {
let char_pos = line[..pos].chars().count() as u64;
char_issues.push((char_pos, "\""));
}
}

if char_issues.is_empty() {
continue;
}

// Build an array of replacements for each character in char_positions
let replacements: Vec<Replacement> = char_issues
.iter()
.map(|&(char_pos, rchar)| Replacement {
deleted_region: Range {
start: Position {
line: i as u64,
character: char_pos,
},
end: Position {
line: i as u64,
character: char_pos + 1,
},
},
inserted_content: rchar.to_string(),
})
.collect();

ret.push(Diagnostic {
path: path.to_str().unwrap().to_string(),
range: Some(Range {
start: Position {
line: i as u64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the replacements 1-indexed and the diagnostics are 0-indexed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If not, the test you added asserts the message, not the diagnostic location

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch - I shoud standardize this in the diagnostics.rs file where we make every 1 index assuming input is 0 except for startLine and endLine...whoops.

character: char_issues.first().unwrap().0,
},
end: Position {
line: i as u64,
character: char_issues.last().unwrap().0 + 1,
},
}),
severity: Severity::Error,
code: "no-curly-quotes".to_string(),
message: format!("Found curly quote on line {}", i + 1),
replacements: Some(replacements),
});
}

Ok(ret)
}
86 changes: 86 additions & 0 deletions tests/no_curly_quote_test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
use spectral::prelude::*;

mod integration_testing;
use integration_testing::TestRepo;

const TOML_ON: &str = r#"
[nocurlyquotes]
enabled = true
"#;

const CURLY_QUOTES: &str = r#"
the opening double quote ( “ ) U+201C
the closing double quote ( ” ) U+201D
the opening single quote ( ‘ ) U+2018
the closing single quote ( ’) U+2019
the double low quotation ( „ ) U+201E
the double high reversed ( ‟ ) U+201F
//
"#;

#[test]
fn honor_disabled_in_config() -> anyhow::Result<()> {
let test_repo: TestRepo = TestRepo::make().unwrap();

test_repo.write("src/curly.txt", "empty_file".as_bytes());
test_repo.git_add_all()?;
test_repo.git_commit_all("create curly quote file");
test_repo.write("src/curly.txt", CURLY_QUOTES.as_bytes());

// disable nocurlyquotes
let toml_off = r#"
[nocurlyquotes]
enabled = false
"#;

test_repo.set_toolbox_toml(TOML_ON);
let mut horton = test_repo.run_horton()?;
assert_that(&horton.has_result("no-curly-quotes", "", Some("src/curly.txt"))).is_true();

test_repo.set_toolbox_toml(toml_off);
horton = test_repo.run_horton()?;

assert_that(&horton.exit_code).contains_value(0);
assert_that(&horton.has_result("no-curly-quotes", "", Some("src/curly.txt"))).is_false();
assert_that(&horton.has_result("toolbox-perf", "1 files processed", None)).is_true();

Ok(())
}

#[test]
fn assert_find_curly_quotes() {
let test_repo = TestRepo::make().unwrap();
test_repo.set_toolbox_toml(TOML_ON);
test_repo.write("revision.foo", "//".as_bytes());
test_repo.git_commit_all("create revision.foo");

{
test_repo.write("revision.foo", CURLY_QUOTES.as_bytes());
let horton = test_repo.run_horton().unwrap();
assert_that(&horton.exit_code).contains_value(0);
assert_that(&horton.has_result(
"no-curly-quotes",
"Found curly quote on line 2",
Some("revision.foo"),
))
.is_true();
assert_that(&horton.has_result(
"no-curly-quotes",
"Found curly quote on line 3",
Some("revision.foo"),
))
.is_true();
assert_that(&horton.has_result(
"no-curly-quotes",
"Found curly quote on line 4",
Some("revision.foo"),
))
.is_true();
assert_that(&horton.has_result(
"no-curly-quotes",
"Found curly quote on line 5",
Some("revision.foo"),
))
.is_true();
}
}
Loading