-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Don't raise D208
when last line is non-empty
#13372
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
fb40db0
Fix indentation error when trailing quotes are not on separate line (…
ukyen8 4bd9266
Use over_indented_size for adjusting indentation
ukyen8 be9f2c4
Convert byte offset to character offset and add comments
ukyen8 4c6db55
Rephrase comments
ukyen8 edc8a37
Don't flag D208 on last line if it contains content
MichaReiser 70723ca
Move linter quick test out of `rules` module because it breaks the co…
MichaReiser File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ use ruff_diagnostics::{AlwaysFixableViolation, Violation}; | |
use ruff_diagnostics::{Diagnostic, Edit, Fix}; | ||
use ruff_macros::{derive_message_formats, violation}; | ||
use ruff_python_ast::docstrings::{clean_space, leading_space}; | ||
use ruff_source_file::NewlineWithTrailingNewline; | ||
use ruff_source_file::{Line, NewlineWithTrailingNewline}; | ||
use ruff_text_size::{Ranged, TextSize}; | ||
use ruff_text_size::{TextLen, TextRange}; | ||
|
||
|
@@ -169,32 +169,49 @@ pub(crate) fn indent(checker: &mut Checker, docstring: &Docstring) { | |
let body = docstring.body(); | ||
|
||
// Split the docstring into lines. | ||
let lines: Vec<_> = NewlineWithTrailingNewline::with_offset(&body, body.start()).collect(); | ||
if lines.len() <= 1 { | ||
let mut lines = NewlineWithTrailingNewline::with_offset(&body, body.start()).peekable(); | ||
|
||
// The current line being processed | ||
let mut current: Option<Line> = lines.next(); | ||
|
||
if lines.peek().is_none() { | ||
return; | ||
} | ||
|
||
let mut has_seen_tab = docstring.indentation.contains('\t'); | ||
let mut is_over_indented = true; | ||
let docstring_indent_size = docstring.indentation.chars().count(); | ||
|
||
// Lines, other than the last, that are over indented. | ||
let mut over_indented_lines = vec![]; | ||
let mut over_indented_size = usize::MAX; | ||
// The smallest over indent that all docstring lines have in common. None if any line isn't over indented. | ||
let mut smallest_over_indent_size = Some(usize::MAX); | ||
// The last processed line | ||
let mut last = None; | ||
|
||
let docstring_indent_size = docstring.indentation.chars().count(); | ||
for i in 0..lines.len() { | ||
// First lines and continuations doesn't need any indentation. | ||
if i == 0 || lines[i - 1].ends_with('\\') { | ||
while let Some(line) = current { | ||
// First lines and continuations don't need any indentation. | ||
if last.is_none() | ||
|| last | ||
.as_deref() | ||
.is_some_and(|last: &str| last.ends_with('\\')) | ||
{ | ||
last = Some(line); | ||
current = lines.next(); | ||
continue; | ||
} | ||
|
||
let line = &lines[i]; | ||
let is_last = lines.peek().is_none(); | ||
|
||
// Omit empty lines, except for the last line, which is non-empty by way of | ||
// containing the closing quotation marks. | ||
let is_blank = line.trim().is_empty(); | ||
if i < lines.len() - 1 && is_blank { | ||
if !is_last && is_blank { | ||
last = Some(line); | ||
current = lines.next(); | ||
continue; | ||
} | ||
|
||
let line_indent = leading_space(line); | ||
let line_indent = leading_space(&line); | ||
let line_indent_size = line_indent.chars().count(); | ||
|
||
// We only report tab indentation once, so only check if we haven't seen a tab | ||
|
@@ -204,7 +221,7 @@ pub(crate) fn indent(checker: &mut Checker, docstring: &Docstring) { | |
if checker.enabled(Rule::UnderIndentation) { | ||
// We report under-indentation on every line. This isn't great, but enables | ||
// fix. | ||
if (i == lines.len() - 1 || !is_blank) && line_indent_size < docstring_indent_size { | ||
if (is_last || !is_blank) && line_indent_size < docstring_indent_size { | ||
let mut diagnostic = | ||
Diagnostic::new(UnderIndentation, TextRange::empty(line.start())); | ||
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( | ||
|
@@ -215,23 +232,35 @@ pub(crate) fn indent(checker: &mut Checker, docstring: &Docstring) { | |
} | ||
} | ||
|
||
// Only true when the last line is indentation only followed by the closing quotes. | ||
// False when it is not the last line or the last line contains content other than the closing quotes. | ||
// The last line only has special handling when it contains no other content. | ||
let is_last_closing_quotes_only = is_last && is_blank; | ||
|
||
// Like pydocstyle, we only report over-indentation if either: (1) every line | ||
// (except, optionally, the last line) is over-indented, or (2) the last line | ||
// (which contains the closing quotation marks) is | ||
// over-indented. We can't know if we've achieved that condition | ||
// until we've viewed all the lines, so for now, just track | ||
// the over-indentation status of every line. | ||
if i < lines.len() - 1 { | ||
if line_indent_size > docstring_indent_size { | ||
over_indented_lines.push(line); | ||
if !is_last_closing_quotes_only { | ||
smallest_over_indent_size = | ||
smallest_over_indent_size.and_then(|smallest_over_indent_size| { | ||
let over_indent_size = line_indent_size.saturating_sub(docstring_indent_size); | ||
|
||
// Track the _smallest_ offset we see, in terms of characters. | ||
over_indented_size = | ||
std::cmp::min(line_indent_size - docstring_indent_size, over_indented_size); | ||
} else { | ||
is_over_indented = false; | ||
} | ||
// `docstring_indent_size < line_indent_size` | ||
if over_indent_size > 0 { | ||
over_indented_lines.push(line.clone()); | ||
// Track the _smallest_ offset we see, in terms of characters. | ||
Some(smallest_over_indent_size.min(over_indent_size)) | ||
} else { | ||
None | ||
} | ||
}); | ||
} | ||
|
||
last = Some(line); | ||
current = lines.next(); | ||
} | ||
|
||
if checker.enabled(Rule::IndentWithSpaces) { | ||
|
@@ -244,9 +273,9 @@ pub(crate) fn indent(checker: &mut Checker, docstring: &Docstring) { | |
|
||
if checker.enabled(Rule::OverIndentation) { | ||
// If every line (except the last) is over-indented... | ||
if is_over_indented { | ||
if let Some(smallest_over_indent_size) = smallest_over_indent_size { | ||
for line in over_indented_lines { | ||
let line_indent = leading_space(line); | ||
let line_indent = leading_space(&line); | ||
let indent = clean_space(docstring.indentation); | ||
|
||
// We report over-indentation on every line. This isn't great, but | ||
|
@@ -275,7 +304,7 @@ pub(crate) fn indent(checker: &mut Checker, docstring: &Docstring) { | |
.locator() | ||
.after(line.start()) | ||
.chars() | ||
.take(docstring.indentation.chars().count() + over_indented_size) | ||
.take(docstring_indent_size + smallest_over_indent_size) | ||
.map(TextLen::text_len) | ||
.sum::<TextSize>(); | ||
let range = TextRange::at(line.start(), offset); | ||
|
@@ -287,10 +316,13 @@ pub(crate) fn indent(checker: &mut Checker, docstring: &Docstring) { | |
} | ||
|
||
// If the last line is over-indented... | ||
if let Some(last) = lines.last() { | ||
let line_indent = leading_space(last); | ||
if let Some(last) = last { | ||
let line_indent = leading_space(&last); | ||
let line_indent_size = line_indent.chars().count(); | ||
if line_indent_size > docstring_indent_size { | ||
let last_line_over_indent = line_indent_size.saturating_sub(docstring_indent_size); | ||
|
||
let is_indent_only = line_indent.len() == last.len(); | ||
if last_line_over_indent > 0 && is_indent_only { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This requires the same logic. Only apply the last line logic when it is the last line and it only contains the quotes |
||
let mut diagnostic = | ||
Diagnostic::new(OverIndentation, TextRange::empty(last.start())); | ||
let indent = clean_space(docstring.indentation); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main fix where we only exclude the last line if it is all empty