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

RUF010: New rule for removing blank comments #4269

Closed
Closed
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
19 changes: 19 additions & 0 deletions crates/ruff/resources/test/fixtures/ruff/RUF010.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Multi-line comment
# with a blank line
#
# is valid

print("Hello, world!") #


def func_1():
# This is a valid comment
print("Hello, world!") #


def func_2():
# Indented multi-line comment
# with a blank line
#
# is valid
print("Hello, world!")
6 changes: 6 additions & 0 deletions crates/ruff/src/checkers/physical_lines.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use crate::rules::pycodestyle::rules::{
use crate::rules::pygrep_hooks::rules::{blanket_noqa, blanket_type_ignore};
use crate::rules::pylint;
use crate::rules::pyupgrade::rules::unnecessary_coding_comment;
use crate::rules::ruff::rules::blank_comment;
use crate::settings::{flags, Settings};

pub fn check_physical_lines(
Expand All @@ -34,6 +35,7 @@ pub fn check_physical_lines(
let mut has_any_shebang = false;

let enforce_blanket_noqa = settings.rules.enabled(Rule::BlanketNOQA);
let enforce_blank_comment = settings.rules.enabled(Rule::BlankComment);
let enforce_shebang_not_executable = settings.rules.enabled(Rule::ShebangNotExecutable);
let enforce_shebang_missing = settings.rules.enabled(Rule::ShebangMissingExecutableFile);
let enforce_shebang_whitespace = settings.rules.enabled(Rule::ShebangLeadingWhitespace);
Expand Down Expand Up @@ -83,6 +85,10 @@ pub fn check_physical_lines(
blanket_noqa(&mut diagnostics, &line);
}

if enforce_blank_comment {
blank_comment(&mut diagnostics, &line, settings, autofix);
}

if enforce_shebang_missing
|| enforce_shebang_not_executable
|| enforce_shebang_whitespace
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
(Ruff, "007") => Rule::PairwiseOverZipped,
(Ruff, "008") => Rule::MutableDataclassDefault,
(Ruff, "009") => Rule::FunctionCallInDataclassDefaultArgument,
(Ruff, "010") => Rule::BlankComment,
(Ruff, "100") => Rule::UnusedNOQA,

// flake8-django
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,7 @@ ruff_macros::register_rules!(
rules::ruff::rules::AmbiguousUnicodeCharacterString,
rules::ruff::rules::AmbiguousUnicodeCharacterDocstring,
rules::ruff::rules::AmbiguousUnicodeCharacterComment,
rules::ruff::rules::BlankComment,
rules::ruff::rules::CollectionLiteralConcatenation,
rules::ruff::rules::AsyncioDanglingTask,
rules::ruff::rules::UnusedNOQA,
Expand Down Expand Up @@ -902,6 +903,7 @@ impl Rule {
| Rule::ShebangLeadingWhitespace
| Rule::TrailingWhitespace
| Rule::TabIndentation
| Rule::BlankComment
| Rule::BlankLineWithWhitespace => LintSource::PhysicalLines,
Rule::AmbiguousUnicodeCharacterComment
| Rule::AmbiguousUnicodeCharacterDocstring
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ mod tests {

#[test_case(Rule::CollectionLiteralConcatenation, Path::new("RUF005.py"); "RUF005")]
#[test_case(Rule::AsyncioDanglingTask, Path::new("RUF006.py"); "RUF006")]
#[test_case(Rule::BlankComment, Path::new("RUF010.py"); "RUF010")]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
63 changes: 63 additions & 0 deletions crates/ruff/src/rules/ruff/rules/blank_comment.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
use crate::registry::Rule;
use crate::settings::{flags, Settings};
use once_cell::sync::Lazy;
use regex::Regex;
use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::newlines::Line;
use ruff_text_size::{TextLen, TextRange, TextSize};

/// ## What it does
/// Check for blank comments.
///
/// ## Why is this bad?
/// Blank comments are useless and should be removed.
///
/// ## Example
/// ```python
/// print("Hello, World!") #
/// ```
///
/// Use instead:
/// ```python
/// print("Hello, World!")
/// ```
///
/// ## References
/// - [Ruff documentation](https://beta.ruff.rs/docs/configuration/#error-suppression)
#[violation]
pub struct BlankComment;

impl AlwaysAutofixableViolation for BlankComment {
#[derive_message_formats]
fn message(&self) -> String {
format!("Blank comments are useless and should be removed")
}

fn autofix_title(&self) -> String {
"Remove blank comment".to_string()
}
}

static BLACK_COMMENT_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"\S(\s*#\s*)$").unwrap());

/// RUF010
pub fn blank_comment(
diagnostics: &mut Vec<Diagnostic>,
line: &Line,
settings: &Settings,
autofix: flags::Autofix,
) {
if let Some(captures) = BLACK_COMMENT_REGEX.captures(line.as_str()) {
Copy link
Member

Choose a reason for hiding this comment

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

Ruff has the Indexer data structure that exposes the range of all comments (Indexer::comment_ranges). You can use this with Locator.slice(range) to retrieve all comments and call the rule at the top level (module) once. This should be faster than running a regex on every line.

I think we should also be able to replace the regex all together by manually skipping the # and then testing if text.trim().is_empty().

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I guess we could do something like this

// Retrieve the comment using Locator.slice
        let comment = Locator.slice(range);

        // Skip the # at the start of the comment and trim white spaces
        let trimmed_comment = comment.strip_prefix("#").unwrap_or("").trim();

        // If the trimmed comment is empty, it's a blank comment
        if trimmed_comment.is_empty() {
            let mut diagnostic = Diagnostic::new(BlankComment, range);
            if autofix.into() && settings.rules.should_fix(Rule::BlankComment) {
                diagnostic.set_fix(Edit::deletion(range.start(), range.end()));
            }
            diagnostics.push(diagnostic);
        }
    }
    

let match_ = captures.get(1).unwrap();
let range = TextRange::at(
line.start() + TextSize::try_from(match_.start()).unwrap(),
match_.as_str().text_len(),
);
let mut diagnostic = Diagnostic::new(BlankComment, range);
if autofix.into() && settings.rules.should_fix(Rule::BlankComment) {
diagnostic.set_fix(Edit::deletion(range.start(), range.end()));
}
diagnostics.push(diagnostic);
}
}
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
mod ambiguous_unicode_character;
mod asyncio_dangling_task;
mod blank_comment;
mod collection_literal_concatenation;
mod mutable_defaults_in_dataclass_fields;
mod pairwise_over_zipped;
Expand All @@ -10,6 +11,7 @@ pub use ambiguous_unicode_character::{
AmbiguousUnicodeCharacterDocstring, AmbiguousUnicodeCharacterString,
};
pub use asyncio_dangling_task::{asyncio_dangling_task, AsyncioDanglingTask};
pub use blank_comment::{blank_comment, BlankComment};
pub use collection_literal_concatenation::{
collection_literal_concatenation, CollectionLiteralConcatenation,
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
---
source: crates/ruff/src/rules/ruff/mod.rs
---
RUF010.py:6:23: RUF010 [*] Blank comments are useless and should be removed
|
6 | # is valid
7 |
8 | print("Hello, world!") #
| ^^^ RUF010
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Can we change the range to only highlight the comment

|
= help: Remove blank comment

ℹ Suggested fix
3 3 | #
4 4 | # is valid
5 5 |
6 |-print("Hello, world!") #
6 |+print("Hello, world!")
7 7 |
8 8 |
9 9 | def func_1():

RUF010.py:11:27: RUF010 [*] Blank comments are useless and should be removed
|
11 | def func_1():
12 | # This is a valid comment
13 | print("Hello, world!") #
| ^^^ RUF010
|
= help: Remove blank comment

ℹ Suggested fix
8 8 |
9 9 | def func_1():
10 10 | # This is a valid comment
11 |- print("Hello, world!") #
11 |+ print("Hello, world!")
12 12 |
13 13 |
14 14 | def func_2():


2 changes: 2 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.