Skip to content

Commit

Permalink
Add task-tags setting
Browse files Browse the repository at this point in the history 
Programmers often leave comments to themselves and others such as:

    # TODO: Use a faster algorithm?

The keywords used to prefix such comments are just a convention and vary
from project to project. Other common keywords include FIXME and HACK.

The keywords in use for the codebase are of interest to ruff because
ruff does also lint comments. For example the ERA lint detects
commented-out code but ignores comments starting with such a keyword.
Previously the ERA lint simply hardcoded the regular expression
TODO|FIXME|XXX to achieve that. This commit introduces a new `task-tags`
setting to make this configurable (and to allow other comment lints to
recognize the same set of keywords).

The term "task tags" has probably been popularized by the Eclipse
IDE.[1] For Python there has been the proposal PEP 350[2], which
referred to such keywords as "codetags". That proposal however has been
rejected. We are choosing the term "task tags" over "code tags" because
the former is more descriptive: a task tag describes a task.

While according to the PEP 350 such keywords are also sometimes used for
non-tasks e.g. NOBUG to describe a well-known problem that will never be
addressed due to design problems or domain limitations, such keywords
are so rare that we are neglecting them here in favor of more
descriptive terminology. The vast majority of such keywords does
describe tasks, so naming the setting "task-tags" is apt.

[1]: https://www.eclipse.org/pdt/help/html/task_tags.htm
[2]: https://peps.python.org/pep-0350/

Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
  • Loading branch information
@not-my-profile @charliermarsh
not-my-profile and charliermarsh committed Jan 5, 2023
1 parent 3400be1 commit 8d56e41
Show file tree
Hide file tree
Showing 10 changed files with 184 additions and 91 deletions.
20 changes: 20 additions & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2232,6 +2232,26 @@ target-version = "py37"
---
#### [`task-tags`](#task-tags)
A list of task tags to recognize (e.g., "TODO", "FIXME", "XXX").
Comments starting with these tags will be ignored by commented-out code
detection (`ERA`).
**Default value**: `["TODO", "FIXME", "XXX"]`
**Type**: `Vec<String>`
**Example usage**:
```toml
[tool.ruff]
task-tags = ["HACK"]
```
---
#### [`unfixable`](#unfixable)
A list of check code prefixes to consider un-autofix-able.
Expand Down
7 changes: 7 additions & 0 deletions flake8_to_ruff/src/converter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,7 @@ mod tests {
src: None,
target_version: None,
unfixable: None,
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
Expand Down Expand Up @@ -451,6 +452,7 @@ mod tests {
src: None,
target_version: None,
unfixable: None,
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
Expand Down Expand Up @@ -512,6 +514,7 @@ mod tests {
src: None,
target_version: None,
unfixable: None,
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
Expand Down Expand Up @@ -573,6 +576,7 @@ mod tests {
src: None,
target_version: None,
unfixable: None,
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
Expand Down Expand Up @@ -634,6 +638,7 @@ mod tests {
src: None,
target_version: None,
unfixable: None,
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
Expand Down Expand Up @@ -704,6 +709,7 @@ mod tests {
src: None,
target_version: None,
unfixable: None,
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
Expand Down Expand Up @@ -768,6 +774,7 @@ mod tests {
src: None,
target_version: None,
unfixable: None,
task_tags: None,
update_check: None,
flake8_annotations: None,
flake8_bandit: None,
Expand Down
10 changes: 10 additions & 0 deletions ruff.schema.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,16 @@
}
]
},
"task-tags": {
"description": "A list of task tags to recognize (e.g., \"TODO\", \"FIXME\", \"XXX\").\n\nComments starting with these tags will be ignored by commented-out code detection (`ERA`).",
"type": [
"array",
"null"
],
"items": {
"type": "string"
}
},
"unfixable": {
"description": "A list of check code prefixes to consider un-autofix-able.",
"type": [
Expand Down
2 changes: 1 addition & 1 deletion src/eradicate/checks.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub fn commented_out_code(
let line = locator.slice_source_code_range(&Range::new(location, end_location));

// Verify that the comment is on its own line, and that it contains code.
if is_standalone_comment(&line) && comment_contains_code(&line) {
if is_standalone_comment(&line) && comment_contains_code(&line, &settings.task_tags[..]) {
let mut check = Check::new(CheckKind::CommentedOutCode, Range::new(start, end));
if matches!(autofix, flags::Autofix::Enabled)
&& settings.fixable.contains(&CheckCode::ERA001)
Expand Down
210 changes: 120 additions & 90 deletions src/eradicate/detection.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use regex::Regex;

static ALLOWLIST_REGEX: Lazy<Regex> = Lazy::new(|| {
Regex::new(
r"^(?i)(?:pylint|pyright|noqa|nosec|type:\s*ignore|fmt:\s*(on|off)|isort:\s*(on|off|skip|skip_file|split|dont-add-imports(:\s*\[.*?])?)|TODO|FIXME|XXX)"
r"^(?i)(?:pylint|pyright|noqa|nosec|type:\s*ignore|fmt:\s*(on|off)|isort:\s*(on|off|skip|skip_file|split|dont-add-imports(:\s*\[.*?])?))"
).unwrap()
});
static BRACKET_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"^[()\[\]{}\s]+$").unwrap());
Expand All @@ -30,7 +30,7 @@ static PARTIAL_DICTIONARY_REGEX: Lazy<Regex> =
static PRINT_RETURN_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"^(print|return)\b\s*").unwrap());

/// Returns `true` if a comment contains Python code.
pub fn comment_contains_code(line: &str) -> bool {
pub fn comment_contains_code(line: &str, task_tags: &[String]) -> bool {
let line = if let Some(line) = line.trim().strip_prefix('#') {
line.trim()
} else {
Expand All @@ -47,6 +47,12 @@ pub fn comment_contains_code(line: &str) -> bool {
return false;
}

if let Some(first) = line.split(&[' ', ':']).next() {
if task_tags.iter().any(|tag| tag == first) {
return false;
}
}

if CODING_COMMENT_REGEX.is_match(line) {
return false;
}
Expand Down Expand Up @@ -97,142 +103,166 @@ mod tests {

#[test]
fn comment_contains_code_basic() {
assert!(comment_contains_code("# x = 1"));
assert!(comment_contains_code("#from foo import eradicate"));
assert!(comment_contains_code("#import eradicate"));
assert!(comment_contains_code(r#"#"key": value,"#));
assert!(comment_contains_code(r#"#"key": "value","#));
assert!(comment_contains_code(r#"#"key": 1 + 1,"#));
assert!(comment_contains_code("#'key': 1 + 1,"));
assert!(comment_contains_code(r#"#"key": {"#));
assert!(comment_contains_code("#}"));
assert!(comment_contains_code("#} )]"));

assert!(!comment_contains_code("#"));
assert!(!comment_contains_code("# This is a (real) comment."));
assert!(!comment_contains_code("# 123"));
assert!(!comment_contains_code("# 123.1"));
assert!(!comment_contains_code("# 1, 2, 3"));
assert!(!comment_contains_code("x = 1 # x = 1"));
assert!(comment_contains_code("# x = 1", &[]));
assert!(comment_contains_code("#from foo import eradicate", &[]));
assert!(comment_contains_code("#import eradicate", &[]));
assert!(comment_contains_code(r#"#"key": value,"#, &[]));
assert!(comment_contains_code(r#"#"key": "value","#, &[]));
assert!(comment_contains_code(r#"#"key": 1 + 1,"#, &[]));
assert!(comment_contains_code("#'key': 1 + 1,", &[]));
assert!(comment_contains_code(r#"#"key": {"#, &[]));
assert!(comment_contains_code("#}", &[]));
assert!(comment_contains_code("#} )]", &[]));

assert!(!comment_contains_code("#", &[]));
assert!(!comment_contains_code("# This is a (real) comment.", &[]));
assert!(!comment_contains_code("# 123", &[]));
assert!(!comment_contains_code("# 123.1", &[]));
assert!(!comment_contains_code("# 1, 2, 3", &[]));
assert!(!comment_contains_code("x = 1 # x = 1", &[]));
assert!(!comment_contains_code(
"# pylint: disable=redefined-outer-name",
&[]
),);
assert!(!comment_contains_code(
"# pylint: disable=redefined-outer-name"
"# Issue #999: This is not code",
&[]
));
assert!(!comment_contains_code("# Issue #999: This is not code"));

// TODO(charlie): This should be `true` under aggressive mode.
assert!(!comment_contains_code("#},"));
assert!(!comment_contains_code("#},", &[]));
}

#[test]
fn comment_contains_code_with_print() {
assert!(comment_contains_code("#print"));
assert!(comment_contains_code("#print(1)"));
assert!(comment_contains_code("#print 1"));
assert!(comment_contains_code("#print", &[]));
assert!(comment_contains_code("#print(1)", &[]));
assert!(comment_contains_code("#print 1", &[]));

assert!(!comment_contains_code("#to print"));
assert!(!comment_contains_code("#to print", &[]));
}

#[test]
fn comment_contains_code_with_return() {
assert!(comment_contains_code("#return x"));
assert!(comment_contains_code("#return x", &[]));

assert!(!comment_contains_code("#to print"));
assert!(!comment_contains_code("#to print", &[]));
}

#[test]
fn comment_contains_code_with_multiline() {
assert!(comment_contains_code("#else:"));
assert!(comment_contains_code("# else : "));
assert!(comment_contains_code(r#"# "foo %d" % \\"#));
assert!(comment_contains_code("#elif True:"));
assert!(comment_contains_code("#x = foo("));
assert!(comment_contains_code("#except Exception:"));

assert!(!comment_contains_code("# this is = to that :("));
assert!(!comment_contains_code("#else"));
assert!(!comment_contains_code("#or else:"));
assert!(!comment_contains_code("#else True:"));
assert!(comment_contains_code("#else:", &[]));
assert!(comment_contains_code("# else : ", &[]));
assert!(comment_contains_code(r#"# "foo %d" % \\"#, &[]));
assert!(comment_contains_code("#elif True:", &[]));
assert!(comment_contains_code("#x = foo(", &[]));
assert!(comment_contains_code("#except Exception:", &[]));

assert!(!comment_contains_code("# this is = to that :(", &[]));
assert!(!comment_contains_code("#else", &[]));
assert!(!comment_contains_code("#or else:", &[]));
assert!(!comment_contains_code("#else True:", &[]));

// Unpacking assignments
assert!(comment_contains_code(
"# user_content_type, _ = TimelineEvent.objects.using(db_alias).get_or_create("
));
"# user_content_type, _ = TimelineEvent.objects.using(db_alias).get_or_create(",
&[]
),);
assert!(comment_contains_code(
"# (user_content_type, _) = TimelineEvent.objects.using(db_alias).get_or_create("
));
"# (user_content_type, _) = TimelineEvent.objects.using(db_alias).get_or_create(",
&[]
),);
assert!(comment_contains_code(
"# ( user_content_type , _ )= TimelineEvent.objects.using(db_alias).get_or_create("
"# ( user_content_type , _ )= TimelineEvent.objects.using(db_alias).get_or_create(",
&[]
));
assert!(comment_contains_code(
"# app_label=\"core\", model=\"user\""
"# app_label=\"core\", model=\"user\"",
&[]
));
assert!(comment_contains_code("# )"));
assert!(comment_contains_code("# )", &[]));

// TODO(charlie): This should be `true` under aggressive mode.
assert!(!comment_contains_code("#def foo():"));
assert!(!comment_contains_code("#def foo():", &[]));
}

#[test]
fn comment_contains_code_with_sentences() {
assert!(!comment_contains_code("#code is good"));
assert!(!comment_contains_code("#code is good", &[]));
}

#[test]
fn comment_contains_code_with_encoding() {
assert!(comment_contains_code("# codings=utf-8"));
assert!(comment_contains_code("# codings=utf-8", &[]));

assert!(!comment_contains_code("# coding=utf-8"));
assert!(!comment_contains_code("#coding= utf-8"));
assert!(!comment_contains_code("# coding: utf-8"));
assert!(!comment_contains_code("# encoding: utf8"));
assert!(!comment_contains_code("# coding=utf-8", &[]));
assert!(!comment_contains_code("#coding= utf-8", &[]));
assert!(!comment_contains_code("# coding: utf-8", &[]));
assert!(!comment_contains_code("# encoding: utf8", &[]));
}

#[test]
fn comment_contains_code_with_default_allowlist() {
assert!(!comment_contains_code("# pylint: disable=A0123"));
assert!(!comment_contains_code("# pylint:disable=A0123"));
assert!(!comment_contains_code("# pylint: disable = A0123"));
assert!(!comment_contains_code("# pylint:disable = A0123"));
assert!(!comment_contains_code("# pyright: reportErrorName=true"));
assert!(!comment_contains_code("# noqa"));
assert!(!comment_contains_code("# NOQA"));
assert!(!comment_contains_code("# noqa: A123"));
assert!(!comment_contains_code("# noqa:A123"));
assert!(!comment_contains_code("# nosec"));
assert!(!comment_contains_code("# fmt: on"));
assert!(!comment_contains_code("# fmt: off"));
assert!(!comment_contains_code("# fmt:on"));
assert!(!comment_contains_code("# fmt:off"));
assert!(!comment_contains_code("# isort: on"));
assert!(!comment_contains_code("# isort:on"));
assert!(!comment_contains_code("# isort: off"));
assert!(!comment_contains_code("# isort:off"));
assert!(!comment_contains_code("# isort: skip"));
assert!(!comment_contains_code("# isort:skip"));
assert!(!comment_contains_code("# isort: skip_file"));
assert!(!comment_contains_code("# isort:skip_file"));
assert!(!comment_contains_code("# isort: split"));
assert!(!comment_contains_code("# isort:split"));
assert!(!comment_contains_code("# isort: dont-add-imports"));
assert!(!comment_contains_code("# isort:dont-add-imports"));
assert!(!comment_contains_code("# pylint: disable=A0123", &[]));
assert!(!comment_contains_code("# pylint:disable=A0123", &[]));
assert!(!comment_contains_code("# pylint: disable = A0123", &[]));
assert!(!comment_contains_code("# pylint:disable = A0123", &[]));
assert!(!comment_contains_code(
"# pyright: reportErrorName=true",
&[]
));
assert!(!comment_contains_code("# noqa", &[]));
assert!(!comment_contains_code("# NOQA", &[]));
assert!(!comment_contains_code("# noqa: A123", &[]));
assert!(!comment_contains_code("# noqa:A123", &[]));
assert!(!comment_contains_code("# nosec", &[]));
assert!(!comment_contains_code("# fmt: on", &[]));
assert!(!comment_contains_code("# fmt: off", &[]));
assert!(!comment_contains_code("# fmt:on", &[]));
assert!(!comment_contains_code("# fmt:off", &[]));
assert!(!comment_contains_code("# isort: on", &[]));
assert!(!comment_contains_code("# isort:on", &[]));
assert!(!comment_contains_code("# isort: off", &[]));
assert!(!comment_contains_code("# isort:off", &[]));
assert!(!comment_contains_code("# isort: skip", &[]));
assert!(!comment_contains_code("# isort:skip", &[]));
assert!(!comment_contains_code("# isort: skip_file", &[]));
assert!(!comment_contains_code("# isort:skip_file", &[]));
assert!(!comment_contains_code("# isort: split", &[]));
assert!(!comment_contains_code("# isort:split", &[]));
assert!(!comment_contains_code("# isort: dont-add-imports", &[]));
assert!(!comment_contains_code("# isort:dont-add-imports", &[]));
assert!(!comment_contains_code(
"# isort: dont-add-imports: [\"import os\"]",
&[]
));
assert!(!comment_contains_code(
"# isort:dont-add-imports: [\"import os\"]",
&[]
));
assert!(!comment_contains_code(
"# isort: dont-add-imports:[\"import os\"]",
&[]
));
assert!(!comment_contains_code(
"# isort: dont-add-imports: [\"import os\"]"
"# isort:dont-add-imports:[\"import os\"]",
&[]
));
assert!(!comment_contains_code("# type: ignore", &[]));
assert!(!comment_contains_code("# type:ignore", &[]));
assert!(!comment_contains_code("# type: ignore[import]", &[]));
assert!(!comment_contains_code("# type:ignore[import]", &[]));
assert!(!comment_contains_code(
"# isort:dont-add-imports: [\"import os\"]"
"# TODO: Do that",
&["TODO".to_string()]
));
assert!(!comment_contains_code(
"# isort: dont-add-imports:[\"import os\"]"
"# FIXME: Fix that",
&["FIXME".to_string()]
));
assert!(!comment_contains_code(
"# isort:dont-add-imports:[\"import os\"]"
"# XXX: What ever",
&["XXX".to_string()]
));
assert!(!comment_contains_code("# type: ignore"));
assert!(!comment_contains_code("# type:ignore"));
assert!(!comment_contains_code("# type: ignore[import]"));
assert!(!comment_contains_code("# type:ignore[import]"));
assert!(!comment_contains_code("# TODO: Do that"));
assert!(!comment_contains_code("# FIXME: Fix that"));
assert!(!comment_contains_code("# XXX: What ever"));
}
}

0 comments on commit 8d56e41

Please sign in to comment.