Python
/
ruff
mirror of https://github.com/astral-sh/ruff
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Remove leading and trailing space length from `Directive` (#5545) 

## Summary

We only need this in one place (when removing the directive), and it
simplifies a lot of details to just compute it there.
This commit is contained in:
Charlie Marsh 2023-07-05 19:03:06 -04:00 committed by GitHub
parent c9e02c52a8
commit e4596ebc35
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 89 additions and 152 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

75
crates/ruff/src/checkers/noqa.rs
View File

@ -94,32 +94,17 @@ pub(crate) fn check_noqa(
if analyze_directives && settings.rules.enabled(Rule::UnusedNOQA) { if analyze_directives && settings.rules.enabled(Rule::UnusedNOQA) {
for line in noqa_directives.lines() { for line in noqa_directives.lines() {
match &line.directive { match &line.directive {
Directive::All(All { Directive::All(All { range }) => {
leading_space_len,
noqa_range,
trailing_space_len,
}) => {
if line.matches.is_empty() { if line.matches.is_empty() {
let mut diagnostic = let mut diagnostic = Diagnostic::new(UnusedNOQA { codes: None }, *range);
Diagnostic::new(UnusedNOQA { codes: None }, *noqa_range);
if settings.rules.should_fix(diagnostic.kind.rule()) { if settings.rules.should_fix(diagnostic.kind.rule()) {
#[allow(deprecated)] #[allow(deprecated)]
diagnostic.set_fix_from_edit(delete_noqa( diagnostic.set_fix_from_edit(delete_noqa(*range, locator));
*leading_space_len,
*noqa_range,
*trailing_space_len,
locator,
));
} }
diagnostics.push(diagnostic); diagnostics.push(diagnostic);
} }
} }
Directive::Codes(Codes { Directive::Codes(Codes { range, codes }) => {
leading_space_len,
noqa_range,
codes,
trailing_space_len,
}) => {
let mut disabled_codes = vec![]; let mut disabled_codes = vec![];
let mut unknown_codes = vec![]; let mut unknown_codes = vec![];
let mut unmatched_codes = vec![]; let mut unmatched_codes = vec![];
@ -174,22 +159,17 @@ pub(crate) fn check_noqa(
.collect(), .collect(),
}), }),
}, },
*noqa_range, *range,
); );
if settings.rules.should_fix(diagnostic.kind.rule()) { if settings.rules.should_fix(diagnostic.kind.rule()) {
if valid_codes.is_empty() { if valid_codes.is_empty() {
#[allow(deprecated)] #[allow(deprecated)]
diagnostic.set_fix_from_edit(delete_noqa( diagnostic.set_fix_from_edit(delete_noqa(*range, locator));
*leading_space_len,
*noqa_range,
*trailing_space_len,
locator,
));
} else { } else {
#[allow(deprecated)] #[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
format!("# noqa: {}", valid_codes.join(", ")), format!("# noqa: {}", valid_codes.join(", ")),
*noqa_range, *range,
))); )));
} }
} }
@ -205,39 +185,46 @@ pub(crate) fn check_noqa(
} }
/// Generate a [`Edit`] to delete a `noqa` directive. /// Generate a [`Edit`] to delete a `noqa` directive.
fn delete_noqa( fn delete_noqa(range: TextRange, locator: &Locator) -> Edit {
leading_space_len: TextSize, let line_range = locator.line_range(range.start());
noqa_range: TextRange,
trailing_space_len: TextSize, // Compute the leading space.
locator: &Locator, let prefix = locator.slice(TextRange::new(line_range.start(), range.start()));
) -> Edit { let leading_space = prefix
let line_range = locator.line_range(noqa_range.start()); .rfind(|c: char| !c.is_whitespace())
.map_or(prefix.len(), |i| prefix.len() - i - 1);
let leading_space_len = TextSize::try_from(leading_space).unwrap();
// Compute the trailing space.
let suffix = locator.slice(TextRange::new(range.end(), line_range.end()));
let trailing_space = suffix
.find(|c: char| !c.is_whitespace())
.map_or(suffix.len(), |i| i);
let trailing_space_len = TextSize::try_from(trailing_space).unwrap();
// Ex) `# noqa` // Ex) `# noqa`
if line_range if line_range
== TextRange::new( == TextRange::new(
noqa_range.start() - leading_space_len, range.start() - leading_space_len,
noqa_range.end() + trailing_space_len, range.end() + trailing_space_len,
) )
{ {
let full_line_end = locator.full_line_end(line_range.end()); let full_line_end = locator.full_line_end(line_range.end());
Edit::deletion(line_range.start(), full_line_end) Edit::deletion(line_range.start(), full_line_end)
} }
// Ex) `x = 1 # noqa` // Ex) `x = 1 # noqa`
else if noqa_range.end() + trailing_space_len == line_range.end() { else if range.end() + trailing_space_len == line_range.end() {
Edit::deletion(noqa_range.start() - leading_space_len, line_range.end()) Edit::deletion(range.start() - leading_space_len, line_range.end())
} }
// Ex) `x = 1 # noqa # type: ignore` // Ex) `x = 1 # noqa # type: ignore`
else if locator.contents()[usize::from(noqa_range.end() + trailing_space_len)..] else if locator.contents()[usize::from(range.end() + trailing_space_len)..].starts_with('#') {
.starts_with('#') Edit::deletion(range.start(), range.end() + trailing_space_len)
{
Edit::deletion(noqa_range.start(), noqa_range.end() + trailing_space_len)
} }
// Ex) `x = 1 # noqa here` // Ex) `x = 1 # noqa here`
else { else {
Edit::deletion( Edit::deletion(
noqa_range.start() + "# ".text_len(), range.start() + "# ".text_len(),
noqa_range.end() + trailing_space_len, range.end() + trailing_space_len,
) )
} }
} }

80
crates/ruff/src/noqa.rs
View File

@ -19,9 +19,7 @@ use crate::registry::{AsRule, Rule, RuleSet};
use crate::rule_redirects::get_redirect_target; use crate::rule_redirects::get_redirect_target;
static NOQA_LINE_REGEX: Lazy<Regex> = Lazy::new(|| { static NOQA_LINE_REGEX: Lazy<Regex> = Lazy::new(|| {
Regex::new( Regex::new(r"(?P<noqa>(?i:# noqa)(?::\s?(?P<codes>[A-Z]+[0-9]+(?:[,\s]+[A-Z]+[0-9]+)*))?)")
r"(?P<leading_spaces>\s*)(?P<noqa>(?i:# noqa)(?::\s?(?P<codes>[A-Z]+[0-9]+(?:[,\s]+[A-Z]+[0-9]+)*))?)(?P<trailing_spaces>\s*)",
)
.unwrap() .unwrap()
}); });
@ -39,14 +37,9 @@ impl<'a> Directive<'a> {
/// Extract the noqa `Directive` from a line of Python source code. /// Extract the noqa `Directive` from a line of Python source code.
pub(crate) fn try_extract(range: TextRange, locator: &'a Locator) -> Option<Self> { pub(crate) fn try_extract(range: TextRange, locator: &'a Locator) -> Option<Self> {
let text = &locator.contents()[range]; let text = &locator.contents()[range];
match NOQA_LINE_REGEX.captures(text) { let caps = NOQA_LINE_REGEX.captures(text)?;
Some(caps) => match ( match (caps.name("noqa"), caps.name("codes")) {
caps.name("leading_spaces"), (Some(noqa), Some(codes)) => {
caps.name("noqa"),
caps.name("codes"),
caps.name("trailing_spaces"),
) {
(Some(leading_spaces), Some(noqa), Some(codes), Some(trailing_spaces)) => {
let codes = codes let codes = codes
.as_str() .as_str()
.split(|c: char| c.is_whitespace() || c == ',') .split(|c: char| c.is_whitespace() || c == ',')
@ -60,55 +53,31 @@ impl<'a> Directive<'a> {
warn!("Expected rule codes on `noqa` directive: \"{line}\""); warn!("Expected rule codes on `noqa` directive: \"{line}\"");
} }
let leading_space_len = leading_spaces.as_str().text_len(); let range = TextRange::at(start, noqa.as_str().text_len());
let noqa_range = TextRange::at(start, noqa.as_str().text_len()); Some(Self::Codes(Codes { range, codes }))
let trailing_space_len = trailing_spaces.as_str().text_len();
Some(Self::Codes(Codes {
leading_space_len,
noqa_range,
trailing_space_len,
codes,
}))
} }
(Some(leading_spaces), Some(noqa), None, Some(trailing_spaces)) => { (Some(noqa), None) => {
let leading_space_len = leading_spaces.as_str().text_len(); let range = TextRange::at(
let noqa_range = TextRange::at(
range.start() + TextSize::try_from(noqa.start()).unwrap(), range.start() + TextSize::try_from(noqa.start()).unwrap(),
noqa.as_str().text_len(), noqa.as_str().text_len(),
); );
let trailing_space_len = trailing_spaces.as_str().text_len(); Some(Self::All(All { range }))
Some(Self::All(All {
leading_space_len,
noqa_range,
trailing_space_len,
}))
} }
_ => None, _ => None,
},
None => None,
} }
} }
} }
#[derive(Debug)] #[derive(Debug)]
pub(crate) struct All { pub(crate) struct All {
/// The length of the leading whitespace before the `noqa` directive.
pub(crate) leading_space_len: TextSize,
/// The range of the `noqa` directive. /// The range of the `noqa` directive.
pub(crate) noqa_range: TextRange, pub(crate) range: TextRange,
/// The length of the trailing whitespace after the `noqa` directive.
pub(crate) trailing_space_len: TextSize,
} }
#[derive(Debug)] #[derive(Debug)]
pub(crate) struct Codes<'a> { pub(crate) struct Codes<'a> {
/// The length of the leading whitespace before the `noqa` directive.
pub(crate) leading_space_len: TextSize,
/// The range of the `noqa` directive. /// The range of the `noqa` directive.
pub(crate) noqa_range: TextRange, pub(crate) range: TextRange,
/// The length of the trailing whitespace after the `noqa` directive.
pub(crate) trailing_space_len: TextSize,
/// The codes that are ignored by the `noqa` directive. /// The codes that are ignored by the `noqa` directive.
pub(crate) codes: Vec<&'a str>, pub(crate) codes: Vec<&'a str>,
} }
@ -132,8 +101,8 @@ pub(crate) fn rule_is_ignored(
let offset = noqa_line_for.resolve(offset); let offset = noqa_line_for.resolve(offset);
let line_range = locator.line_range(offset); let line_range = locator.line_range(offset);
match Directive::try_extract(line_range, locator) { match Directive::try_extract(line_range, locator) {
Some(Directive::All(..)) => true, Some(Directive::All(_)) => true,
Some(Directive::Codes(Codes { codes, .. })) => includes(code, &codes), Some(Directive::Codes(Codes { codes, range: _ })) => includes(code, &codes),
None => false, None => false,
} }
} }
@ -289,10 +258,10 @@ fn add_noqa_inner(
directives.find_line_with_directive(noqa_line_for.resolve(parent)) directives.find_line_with_directive(noqa_line_for.resolve(parent))
{ {
match &directive_line.directive { match &directive_line.directive {
Directive::All(..) => { Directive::All(_) => {
continue; continue;
} }
Directive::Codes(Codes { codes, .. }) => { Directive::Codes(Codes { codes, range: _ }) => {
if includes(diagnostic.kind.rule(), codes) { if includes(diagnostic.kind.rule(), codes) {
continue; continue;
} }
@ -306,10 +275,10 @@ fn add_noqa_inner(
// Or ignored by the directive itself? // Or ignored by the directive itself?
if let Some(directive_line) = directives.find_line_with_directive(noqa_offset) { if let Some(directive_line) = directives.find_line_with_directive(noqa_offset) {
match &directive_line.directive { match &directive_line.directive {
Directive::All(..) => { Directive::All(_) => {
continue; continue;
} }
Directive::Codes(Codes { codes, .. }) => { Directive::Codes(Codes { codes, range: _ }) => {
let rule = diagnostic.kind.rule(); let rule = diagnostic.kind.rule();
if !includes(rule, codes) { if !includes(rule, codes) {
matches_by_line matches_by_line
@ -355,12 +324,10 @@ fn add_noqa_inner(
output.push_str(&line_ending); output.push_str(&line_ending);
count += 1; count += 1;
} }
Some(Directive::All(..)) => { Some(Directive::All(_)) => {
// Does not get inserted into the map. // Does not get inserted into the map.
} }
Some(Directive::Codes(Codes { Some(Directive::Codes(Codes { range, codes })) => {
noqa_range, codes, ..
})) => {
// Reconstruct the line based on the preserved rule codes. // Reconstruct the line based on the preserved rule codes.
// This enables us to tally the number of edits. // This enables us to tally the number of edits.
let output_start = output.len(); let output_start = output.len();
@ -368,7 +335,7 @@ fn add_noqa_inner(
// Add existing content. // Add existing content.
output.push_str( output.push_str(
locator locator
.slice(TextRange::new(offset, noqa_range.start())) .slice(TextRange::new(offset, range.start()))
.trim_end(), .trim_end(),
); );
@ -433,12 +400,11 @@ impl<'a> NoqaDirectives<'a> {
) -> Self { ) -> Self {
let mut directives = Vec::new(); let mut directives = Vec::new();
for comment_range in comment_ranges { for range in comment_ranges {
let line_range = locator.line_range(comment_range.start()); if let Some(directive) = Directive::try_extract(*range, locator) {
if let Some(directive) = Directive::try_extract(line_range, locator) {
// noqa comments are guaranteed to be single line. // noqa comments are guaranteed to be single line.
directives.push(NoqaDirectiveLine { directives.push(NoqaDirectiveLine {
range: line_range, range: locator.line_range(range.start()),
directive, directive,
matches: Vec::new(), matches: Vec::new(),
}); });

6
crates/ruff/src/snapshots/ruff__noqa__tests__noqa_all.snap
View File

@ -1,13 +1,11 @@
--- ---
source: crates/ruff/src/noqa.rs source: crates/ruff/src/noqa.rs
expression: "Directive::extract(range, &locator)" expression: "Directive::try_extract(range, &locator)"
--- ---
Some( Some(
All( All(
All { All {
leading_space_len: 0, range: 0..6,
noqa_range: 0..6,
trailing_space_len: 0,
}, },
), ),
) )

6
crates/ruff/src/snapshots/ruff__noqa__tests__noqa_all_case_insensitive.snap
View File

@ -1,13 +1,11 @@
--- ---
source: crates/ruff/src/noqa.rs source: crates/ruff/src/noqa.rs
expression: "Directive::extract(range, &locator)" expression: "Directive::try_extract(range, &locator)"
--- ---
Some( Some(
All( All(
All { All {
leading_space_len: 0, range: 0..6,
noqa_range: 0..6,
trailing_space_len: 0,
}, },
), ),
) )

6
crates/ruff/src/snapshots/ruff__noqa__tests__noqa_code.snap
View File

@ -1,13 +1,11 @@
--- ---
source: crates/ruff/src/noqa.rs source: crates/ruff/src/noqa.rs
expression: "Directive::extract(range, &locator)" expression: "Directive::try_extract(range, &locator)"
--- ---
Some( Some(
Codes( Codes(
Codes { Codes {
leading_space_len: 0, range: 0..12,
noqa_range: 0..12,
trailing_space_len: 0,
codes: [ codes: [
"F401", "F401",
], ],

6
crates/ruff/src/snapshots/ruff__noqa__tests__noqa_code_case_insensitive.snap
View File

@ -1,13 +1,11 @@
--- ---
source: crates/ruff/src/noqa.rs source: crates/ruff/src/noqa.rs
expression: "Directive::extract(range, &locator)" expression: "Directive::try_extract(range, &locator)"
--- ---
Some( Some(
Codes( Codes(
Codes { Codes {
leading_space_len: 0, range: 0..12,
noqa_range: 0..12,
trailing_space_len: 0,
codes: [ codes: [
"F401", "F401",
], ],

6
crates/ruff/src/snapshots/ruff__noqa__tests__noqa_codes.snap
View File

@ -1,13 +1,11 @@
--- ---
source: crates/ruff/src/noqa.rs source: crates/ruff/src/noqa.rs
expression: "Directive::extract(range, &locator)" expression: "Directive::try_extract(range, &locator)"
--- ---
Some( Some(
Codes( Codes(
Codes { Codes {
leading_space_len: 0, range: 0..18,
noqa_range: 0..18,
trailing_space_len: 0,
codes: [ codes: [
"F401", "F401",
"F841", "F841",

6
crates/ruff/src/snapshots/ruff__noqa__tests__noqa_codes_case_insensitive.snap
View File

@ -1,13 +1,11 @@
--- ---
source: crates/ruff/src/noqa.rs source: crates/ruff/src/noqa.rs
expression: "Directive::extract(range, &locator)" expression: "Directive::try_extract(range, &locator)"
--- ---
Some( Some(
Codes( Codes(
Codes { Codes {
leading_space_len: 0, range: 0..18,
noqa_range: 0..18,
trailing_space_len: 0,
codes: [ codes: [
"F401", "F401",
"F841", "F841",

6
crates/ruff/src/snapshots/ruff__noqa__tests__noqa_leading_space.snap
View File

@ -1,13 +1,11 @@
--- ---
source: crates/ruff/src/noqa.rs source: crates/ruff/src/noqa.rs
expression: "Directive::extract(range, &locator)" expression: "Directive::try_extract(range, &locator)"
--- ---
Some( Some(
Codes( Codes(
Codes { Codes {
leading_space_len: 3, range: 4..16,
noqa_range: 4..16,
trailing_space_len: 0,
codes: [ codes: [
"F401", "F401",
], ],

6
crates/ruff/src/snapshots/ruff__noqa__tests__noqa_trailing_space.snap
View File

@ -1,13 +1,11 @@
--- ---
source: crates/ruff/src/noqa.rs source: crates/ruff/src/noqa.rs
expression: "Directive::extract(range, &locator)" expression: "Directive::try_extract(range, &locator)"
--- ---
Some( Some(
Codes( Codes(
Codes { Codes {
leading_space_len: 0, range: 0..12,
noqa_range: 0..12,
trailing_space_len: 3,
codes: [ codes: [
"F401", "F401",
], ],