From 8d56e412ef13f111acc892e78c2fb4e3e1b45cba Mon Sep 17 00:00:00 2001 From: Martin Fischer Date: Tue, 3 Jan 2023 23:27:38 +0100 Subject: [PATCH] Add task-tags setting 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 --- README.md | 20 +++ flake8_to_ruff/src/converter.rs | 7 ++ ruff.schema.json | 10 ++ src/eradicate/checks.rs | 2 +- src/eradicate/detection.rs | 210 ++++++++++++++++++-------------- src/lib_wasm.rs | 1 + src/settings/configuration.rs | 3 + src/settings/mod.rs | 6 + src/settings/options.rs | 10 ++ src/settings/pyproject.rs | 6 + 10 files changed, 184 insertions(+), 91 deletions(-) diff --git a/README.md b/README.md index 3685509cbc..c0c3f33898 100644 --- a/README.md +++ b/README.md @@ -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` + +**Example usage**: + +```toml +[tool.ruff] +task-tags = ["HACK"] +``` + +--- + #### [`unfixable`](#unfixable) A list of check code prefixes to consider un-autofix-able. diff --git a/flake8_to_ruff/src/converter.rs b/flake8_to_ruff/src/converter.rs index 1154199816..f563575d6b 100644 --- a/flake8_to_ruff/src/converter.rs +++ b/flake8_to_ruff/src/converter.rs @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, diff --git a/ruff.schema.json b/ruff.schema.json index fab5b52b0e..e4ab5b8d6d 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -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": [ diff --git a/src/eradicate/checks.rs b/src/eradicate/checks.rs index 5cefb29b3a..a47604a769 100644 --- a/src/eradicate/checks.rs +++ b/src/eradicate/checks.rs @@ -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) diff --git a/src/eradicate/detection.rs b/src/eradicate/detection.rs index 4d368f9800..68b996549e 100644 --- a/src/eradicate/detection.rs +++ b/src/eradicate/detection.rs @@ -4,7 +4,7 @@ use regex::Regex; static ALLOWLIST_REGEX: Lazy = 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 = Lazy::new(|| Regex::new(r"^[()\[\]{}\s]+$").unwrap()); @@ -30,7 +30,7 @@ static PARTIAL_DICTIONARY_REGEX: Lazy = static PRINT_RETURN_REGEX: Lazy = 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 { @@ -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; } @@ -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("# 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("#", &[])); + 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" + "# pylint: disable=redefined-outer-name", + &[] + ),); + assert!(!comment_contains_code( + "# 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("#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("# 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(", + &[] + ),); + assert!(comment_contains_code( + "# ( 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(" + "# app_label=\"core\", model=\"user\"", + &[] )); - assert!(comment_contains_code( - "# ( user_content_type , _ )= TimelineEvent.objects.using(db_alias).get_or_create(" - )); - assert!(comment_contains_code( - "# 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( - "# isort: dont-add-imports: [\"import os\"]" + "# 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\"]" + "# 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( - "# 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( + "# TODO: Do that", + &["TODO".to_string()] + )); + assert!(!comment_contains_code( + "# FIXME: Fix that", + &["FIXME".to_string()] + )); + assert!(!comment_contains_code( + "# 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")); } } diff --git a/src/lib_wasm.rs b/src/lib_wasm.rs index 30a0eabe5f..d042cbb26f 100644 --- a/src/lib_wasm.rs +++ b/src/lib_wasm.rs @@ -112,6 +112,7 @@ pub fn defaultSettings() -> Result { show_source: None, src: None, unfixable: None, + task_tags: None, update_check: None, // Use default options for all plugins. flake8_annotations: Some(flake8_annotations::settings::Settings::default().into()), diff --git a/src/settings/configuration.rs b/src/settings/configuration.rs index 077b119035..f9faa0213d 100644 --- a/src/settings/configuration.rs +++ b/src/settings/configuration.rs @@ -52,6 +52,7 @@ pub struct Configuration { pub src: Option>, pub target_version: Option, pub unfixable: Option>, + pub task_tags: Option>, pub update_check: Option, // Plugins pub flake8_annotations: Option, @@ -150,6 +151,7 @@ impl Configuration { .transpose()?, target_version: options.target_version, unfixable: options.unfixable, + task_tags: options.task_tags, update_check: options.update_check, // Plugins flake8_annotations: options.flake8_annotations, @@ -211,6 +213,7 @@ impl Configuration { src: self.src.or(config.src), target_version: self.target_version.or(config.target_version), unfixable: self.unfixable.or(config.unfixable), + task_tags: self.task_tags.or(config.task_tags), update_check: self.update_check.or(config.update_check), // Plugins flake8_annotations: self.flake8_annotations.or(config.flake8_annotations), diff --git a/src/settings/mod.rs b/src/settings/mod.rs index bb37d4a84f..6e81554370 100644 --- a/src/settings/mod.rs +++ b/src/settings/mod.rs @@ -60,6 +60,7 @@ pub struct Settings { pub show_source: bool, pub src: Vec, pub target_version: PythonVersion, + pub task_tags: Vec, pub update_check: bool, // Plugins pub flake8_annotations: flake8_annotations::settings::Settings, @@ -174,6 +175,9 @@ impl Settings { .src .unwrap_or_else(|| vec![project_root.to_path_buf()]), target_version: config.target_version.unwrap_or_default(), + task_tags: config.task_tags.unwrap_or_else(|| { + vec!["TODO".to_string(), "FIXME".to_string(), "XXX".to_string()] + }), update_check: config.update_check.unwrap_or(true), // Plugins flake8_annotations: config @@ -257,6 +261,7 @@ impl Settings { show_source: false, src: vec![path_dedot::CWD.clone()], target_version: PythonVersion::Py310, + task_tags: vec!["TODO".to_string(), "FIXME".to_string()], update_check: false, flake8_annotations: flake8_annotations::settings::Settings::default(), flake8_bandit: flake8_bandit::settings::Settings::default(), @@ -297,6 +302,7 @@ impl Settings { show_source: false, src: vec![path_dedot::CWD.clone()], target_version: PythonVersion::Py310, + task_tags: vec!["TODO".to_string()], update_check: false, flake8_annotations: flake8_annotations::settings::Settings::default(), flake8_bandit: flake8_bandit::settings::Settings::default(), diff --git a/src/settings/options.rs b/src/settings/options.rs index 1bf3e5761f..330bbb90ae 100644 --- a/src/settings/options.rs +++ b/src/settings/options.rs @@ -339,6 +339,16 @@ pub struct Options { )] /// A list of check code prefixes to consider un-autofix-able. pub unfixable: Option>, + #[option( + default = r#"["TODO", "FIXME", "XXX"]"#, + value_type = "Vec", + example = r#"task-tags = ["HACK"]"# + )] + /// 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`). + pub task_tags: Option>, #[option( default = "true", value_type = "bool", diff --git a/src/settings/pyproject.rs b/src/settings/pyproject.rs index 1cec9e73f4..cc7cfe9dc8 100644 --- a/src/settings/pyproject.rs +++ b/src/settings/pyproject.rs @@ -188,6 +188,7 @@ mod tests { src: None, target_version: None, unfixable: None, + task_tags: None, update_check: None, flake8_annotations: None, flake8_bandit: None, @@ -242,6 +243,7 @@ line-length = 79 src: None, target_version: None, unfixable: None, + task_tags: None, update_check: None, cache_dir: None, flake8_annotations: None, @@ -298,6 +300,7 @@ exclude = ["foo.py"] src: None, target_version: None, unfixable: None, + task_tags: None, update_check: None, flake8_annotations: None, flake8_bandit: None, @@ -353,6 +356,7 @@ select = ["E501"] src: None, target_version: None, unfixable: None, + task_tags: None, update_check: None, flake8_annotations: None, flake8_bandit: None, @@ -409,6 +413,7 @@ ignore = ["E501"] src: None, target_version: None, unfixable: None, + task_tags: None, update_check: None, flake8_annotations: None, flake8_bandit: None, @@ -495,6 +500,7 @@ other-attribute = 1 format: None, force_exclude: None, unfixable: None, + task_tags: None, update_check: None, cache_dir: None, per_file_ignores: Some(FxHashMap::from_iter([(