From 741e180e2d30078c59e03a8761d90757964c5cf7 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Thu, 25 May 2023 09:51:45 -0500 Subject: [PATCH] Change TODO directive detection to work with multiple pound signs on the same line (#4558) --- .../test/fixtures/flake8_todos/TD001.py | 1 + .../test/fixtures/flake8_todos/TD002.py | 1 + .../test/fixtures/flake8_todos/TD003.py | 2 + .../test/fixtures/flake8_todos/TD004.py | 2 + .../test/fixtures/flake8_todos/TD005.py | 1 + .../test/fixtures/flake8_todos/TD006.py | 1 + .../test/fixtures/flake8_todos/TD007.py | 2 + .../src/rules/flake8_builtins/rules/rules.rs | 2 - .../src/rules/flake8_todos/rules/todos.rs | 91 +++++++++++-------- ..._invalid-todo-capitalization_TD006.py.snap | 20 ++++ ...dos__tests__invalid-todo-tag_TD001.py.snap | 22 +++-- ...ssing-space-after-todo-colon_TD007.py.snap | 12 +++ ...__tests__missing-todo-author_TD002.py.snap | 36 +++++--- ...s__tests__missing-todo-colon_TD004.py.snap | 32 +++++-- ...ts__missing-todo-description_TD005.py.snap | 10 ++ ...os__tests__missing-todo-link_TD003.py.snap | 14 ++- 16 files changed, 182 insertions(+), 67 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_todos/TD001.py b/crates/ruff/resources/test/fixtures/flake8_todos/TD001.py index 542db85c72491..77ff4ae70d081 100644 --- a/crates/ruff/resources/test/fixtures/flake8_todos/TD001.py +++ b/crates/ruff/resources/test/fixtures/flake8_todos/TD001.py @@ -6,3 +6,4 @@ # T001 - errors # XXX (evanrittenhouse): this is not fine # FIXME (evanrittenhouse): this is not fine +# foo # XXX: this isn't fine either diff --git a/crates/ruff/resources/test/fixtures/flake8_todos/TD002.py b/crates/ruff/resources/test/fixtures/flake8_todos/TD002.py index bab01176110d4..3c4867516f783 100644 --- a/crates/ruff/resources/test/fixtures/flake8_todos/TD002.py +++ b/crates/ruff/resources/test/fixtures/flake8_todos/TD002.py @@ -5,3 +5,4 @@ # TODO: this has no author # FIXME: neither does this # TODO : and neither does this +# foo # TODO: this doesn't either diff --git a/crates/ruff/resources/test/fixtures/flake8_todos/TD003.py b/crates/ruff/resources/test/fixtures/flake8_todos/TD003.py index af00d6c82a1ab..f203bfe4f6605 100644 --- a/crates/ruff/resources/test/fixtures/flake8_todos/TD003.py +++ b/crates/ruff/resources/test/fixtures/flake8_todos/TD003.py @@ -26,4 +26,6 @@ def foo(x): # TODO: followed by a new TODO with an issue link # TDO-3870 +# foo # TODO: no link! + # TODO: here's a TODO on the last line with no link diff --git a/crates/ruff/resources/test/fixtures/flake8_todos/TD004.py b/crates/ruff/resources/test/fixtures/flake8_todos/TD004.py index 50dd9a78ad814..9fa5d9c50a105 100644 --- a/crates/ruff/resources/test/fixtures/flake8_todos/TD004.py +++ b/crates/ruff/resources/test/fixtures/flake8_todos/TD004.py @@ -4,3 +4,5 @@ # TODO this has no colon # TODO(evanrittenhouse 😀) this has no colon # FIXME add a colon +# foo # TODO add a colon +# TODO this has a colon but it doesn't terminate the tag, so this should throw. https://www.google.com diff --git a/crates/ruff/resources/test/fixtures/flake8_todos/TD005.py b/crates/ruff/resources/test/fixtures/flake8_todos/TD005.py index a16bb51de65b8..64e8fe5364ab7 100644 --- a/crates/ruff/resources/test/fixtures/flake8_todos/TD005.py +++ b/crates/ruff/resources/test/fixtures/flake8_todos/TD005.py @@ -4,3 +4,4 @@ # TODO(evanrittenhouse): # TODO(evanrittenhouse) # FIXME +# foo # TODO diff --git a/crates/ruff/resources/test/fixtures/flake8_todos/TD006.py b/crates/ruff/resources/test/fixtures/flake8_todos/TD006.py index 3288e23f56c61..90fbbe387b078 100644 --- a/crates/ruff/resources/test/fixtures/flake8_todos/TD006.py +++ b/crates/ruff/resources/test/fixtures/flake8_todos/TD006.py @@ -3,3 +3,4 @@ # TDO006 - error # ToDo (evanrittenhouse): invalid capitalization # todo (evanrittenhouse): another invalid capitalization +# foo # todo: invalid capitalization diff --git a/crates/ruff/resources/test/fixtures/flake8_todos/TD007.py b/crates/ruff/resources/test/fixtures/flake8_todos/TD007.py index 7283dbb60a297..44915d6eda5c1 100644 --- a/crates/ruff/resources/test/fixtures/flake8_todos/TD007.py +++ b/crates/ruff/resources/test/fixtures/flake8_todos/TD007.py @@ -6,3 +6,5 @@ # TODO (evanrittenhouse):this doesn't either # TODO:neither does this # FIXME:and lastly neither does this +# foo # TODO:this is really the last one +# TODO this colon doesn't terminate the tag, so don't check it. https://www.google.com diff --git a/crates/ruff/src/rules/flake8_builtins/rules/rules.rs b/crates/ruff/src/rules/flake8_builtins/rules/rules.rs index b28b04f643122..8b137891791fe 100644 --- a/crates/ruff/src/rules/flake8_builtins/rules/rules.rs +++ b/crates/ruff/src/rules/flake8_builtins/rules/rules.rs @@ -1,3 +1 @@ - - diff --git a/crates/ruff/src/rules/flake8_todos/rules/todos.rs b/crates/ruff/src/rules/flake8_todos/rules/todos.rs index 2eda0f70696dd..d641a717612d2 100644 --- a/crates/ruff/src/rules/flake8_todos/rules/todos.rs +++ b/crates/ruff/src/rules/flake8_todos/rules/todos.rs @@ -230,33 +230,42 @@ enum Directive { impl Directive { /// Extract a [`Directive`] from a comment. /// - /// Returns the offset of the directive within the comment, and the matching directive tag. + /// Returns the matching directive tag and its offset within the comment. fn from_comment(comment: &str) -> Option<(Directive, TextSize)> { - let trimmed = comment.trim_start_matches('#').trim_start(); - let offset = comment.text_len() - trimmed.text_len(); - let mut chars = trimmed.chars(); - match ( - chars.next(), - chars.next(), - chars.next(), - chars.next(), - chars.next(), - ) { - ( - Some('F' | 'f'), - Some('I' | 'i'), - Some('X' | 'x'), - Some('M' | 'm'), - Some('E' | 'e'), - ) => Some((Directive::Fixme, offset)), - (Some('T' | 't'), Some('O' | 'o'), Some('D' | 'd'), Some('O' | 'o'), ..) => { - Some((Directive::Todo, offset)) - } - (Some('X' | 'x'), Some('X' | 'x'), Some('X' | 'x'), ..) => { - Some((Directive::Xxx, offset)) + let mut subset_opt = Some(comment); + let mut total_offset = TextSize::new(0); + + // Loop over the comment to catch cases like `# foo # TODO`. + while let Some(subset) = subset_opt { + let trimmed = subset.trim_start_matches('#').trim_start().to_lowercase(); + + let offset = subset.text_len() - trimmed.text_len(); + total_offset += offset; + + let directive = if trimmed.starts_with("fixme") { + Some((Directive::Fixme, total_offset)) + } else if trimmed.starts_with("xxx") { + Some((Directive::Xxx, total_offset)) + } else if trimmed.starts_with("todo") { + Some((Directive::Todo, total_offset)) + } else { + None + }; + + if directive.is_some() { + return directive; } - _ => None, + + // Shrink the subset to check for the next phrase starting with "#". + subset_opt = if let Some(new_offset) = trimmed.find('#') { + total_offset += TextSize::try_from(new_offset).unwrap(); + subset.get(total_offset.to_usize()..) + } else { + None + }; } + + None } /// Returns the length of the directive tag. @@ -412,25 +421,19 @@ fn static_errors( TextSize::new(0) }; - let post_author = &post_tag[usize::from(author_end)..]; - - let post_colon = if let Some((.., after_colon)) = post_author.split_once(':') { - if let Some(stripped) = after_colon.strip_prefix(' ') { - stripped - } else { - // TD-007 + let after_author = &post_tag[usize::from(author_end)..]; + if let Some(after_colon) = after_author.strip_prefix(':') { + if after_colon.is_empty() { + diagnostics.push(Diagnostic::new(MissingTodoDescription, tag.range)); + } else if !after_colon.starts_with(char::is_whitespace) { diagnostics.push(Diagnostic::new(MissingSpaceAfterTodoColon, tag.range)); - after_colon } } else { - // TD-004 diagnostics.push(Diagnostic::new(MissingTodoColon, tag.range)); - "" - }; - if post_colon.is_empty() { - // TD-005 - diagnostics.push(Diagnostic::new(MissingTodoDescription, tag.range)); + if after_author.is_empty() { + diagnostics.push(Diagnostic::new(MissingTodoDescription, tag.range)); + } } } @@ -466,5 +469,17 @@ mod tests { range: TextRange::new(TextSize::new(2), TextSize::new(7)), }; assert_eq!(Some(expected), detect_tag(test_comment, TextSize::new(0))); + let test_comment = "# noqa # TODO: todo"; + let expected = Tag { + content: "TODO", + range: TextRange::new(TextSize::new(9), TextSize::new(13)), + }; + assert_eq!(Some(expected), detect_tag(test_comment, TextSize::new(0))); + let test_comment = "# noqa # XXX"; + let expected = Tag { + content: "XXX", + range: TextRange::new(TextSize::new(9), TextSize::new(12)), + }; + assert_eq!(Some(expected), detect_tag(test_comment, TextSize::new(0))); } } diff --git a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__invalid-todo-capitalization_TD006.py.snap b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__invalid-todo-capitalization_TD006.py.snap index 7c87a41fc45dd..1806934b04b46 100644 --- a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__invalid-todo-capitalization_TD006.py.snap +++ b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__invalid-todo-capitalization_TD006.py.snap @@ -8,6 +8,7 @@ TD006.py:4:3: TD006 [*] Invalid TODO capitalization: `ToDo` should be `TODO` 6 | # ToDo (evanrittenhouse): invalid capitalization | ^^^^ TD006 7 | # todo (evanrittenhouse): another invalid capitalization +8 | # foo # todo: invalid capitalization | = help: Replace `ToDo` with `TODO` @@ -18,6 +19,7 @@ TD006.py:4:3: TD006 [*] Invalid TODO capitalization: `ToDo` should be `TODO` 4 |-# ToDo (evanrittenhouse): invalid capitalization 4 |+# TODO (evanrittenhouse): invalid capitalization 5 5 | # todo (evanrittenhouse): another invalid capitalization +6 6 | # foo # todo: invalid capitalization TD006.py:5:3: TD006 [*] Invalid TODO capitalization: `todo` should be `TODO` | @@ -25,6 +27,7 @@ TD006.py:5:3: TD006 [*] Invalid TODO capitalization: `todo` should be `TODO` 6 | # ToDo (evanrittenhouse): invalid capitalization 7 | # todo (evanrittenhouse): another invalid capitalization | ^^^^ TD006 +8 | # foo # todo: invalid capitalization | = help: Replace `todo` with `TODO` @@ -34,5 +37,22 @@ TD006.py:5:3: TD006 [*] Invalid TODO capitalization: `todo` should be `TODO` 4 4 | # ToDo (evanrittenhouse): invalid capitalization 5 |-# todo (evanrittenhouse): another invalid capitalization 5 |+# TODO (evanrittenhouse): another invalid capitalization +6 6 | # foo # todo: invalid capitalization + +TD006.py:6:9: TD006 [*] Invalid TODO capitalization: `todo` should be `TODO` + | +6 | # ToDo (evanrittenhouse): invalid capitalization +7 | # todo (evanrittenhouse): another invalid capitalization +8 | # foo # todo: invalid capitalization + | ^^^^ TD006 + | + = help: Replace `todo` with `TODO` + +ℹ Fix +3 3 | # TDO006 - error +4 4 | # ToDo (evanrittenhouse): invalid capitalization +5 5 | # todo (evanrittenhouse): another invalid capitalization +6 |-# foo # todo: invalid capitalization + 6 |+# foo # TODO: invalid capitalization diff --git a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__invalid-todo-tag_TD001.py.snap b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__invalid-todo-tag_TD001.py.snap index 8631a8f2c728a..b0f31abd45429 100644 --- a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__invalid-todo-tag_TD001.py.snap +++ b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__invalid-todo-tag_TD001.py.snap @@ -2,12 +2,13 @@ source: crates/ruff/src/rules/flake8_todos/mod.rs --- TD001.py:7:3: TD001 Invalid TODO tag: `XXX` - | -7 | # T001 - errors -8 | # XXX (evanrittenhouse): this is not fine - | ^^^ TD001 -9 | # FIXME (evanrittenhouse): this is not fine - | + | + 7 | # T001 - errors + 8 | # XXX (evanrittenhouse): this is not fine + | ^^^ TD001 + 9 | # FIXME (evanrittenhouse): this is not fine +10 | # foo # XXX: this isn't fine either + | TD001.py:8:3: TD001 Invalid TODO tag: `FIXME` | @@ -15,6 +16,15 @@ TD001.py:8:3: TD001 Invalid TODO tag: `FIXME` 9 | # XXX (evanrittenhouse): this is not fine 10 | # FIXME (evanrittenhouse): this is not fine | ^^^^^ TD001 +11 | # foo # XXX: this isn't fine either + | + +TD001.py:9:9: TD001 Invalid TODO tag: `XXX` + | + 9 | # XXX (evanrittenhouse): this is not fine +10 | # FIXME (evanrittenhouse): this is not fine +11 | # foo # XXX: this isn't fine either + | ^^^ TD001 | diff --git a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-space-after-todo-colon_TD007.py.snap b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-space-after-todo-colon_TD007.py.snap index c569e2561b832..ae1fd2d79b136 100644 --- a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-space-after-todo-colon_TD007.py.snap +++ b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-space-after-todo-colon_TD007.py.snap @@ -28,6 +28,7 @@ TD007.py:7:3: TD007 Missing space after colon in TODO 9 | # TODO:neither does this | ^^^^ TD007 10 | # FIXME:and lastly neither does this +11 | # foo # TODO:this is really the last one | TD007.py:8:3: TD007 Missing space after colon in TODO @@ -36,6 +37,17 @@ TD007.py:8:3: TD007 Missing space after colon in TODO 9 | # TODO:neither does this 10 | # FIXME:and lastly neither does this | ^^^^^ TD007 +11 | # foo # TODO:this is really the last one +12 | # TODO this colon doesn't terminate the tag, so don't check it. https://www.google.com + | + +TD007.py:9:9: TD007 Missing space after colon in TODO + | + 9 | # TODO:neither does this +10 | # FIXME:and lastly neither does this +11 | # foo # TODO:this is really the last one + | ^^^^ TD007 +12 | # TODO this colon doesn't terminate the tag, so don't check it. https://www.google.com | diff --git a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-author_TD002.py.snap b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-author_TD002.py.snap index 4a97aebc03f62..1d1f80abceb91 100644 --- a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-author_TD002.py.snap +++ b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-author_TD002.py.snap @@ -12,20 +12,30 @@ TD002.py:5:3: TD002 Missing author in TODO; try: `# TODO(): ...` | TD002.py:6:3: TD002 Missing author in TODO; try: `# TODO(): ...` - | -6 | # T002 - errors -7 | # TODO: this has no author -8 | # FIXME: neither does this - | ^^^^^ TD002 -9 | # TODO : and neither does this - | + | + 6 | # T002 - errors + 7 | # TODO: this has no author + 8 | # FIXME: neither does this + | ^^^^^ TD002 + 9 | # TODO : and neither does this +10 | # foo # TODO: this doesn't either + | TD002.py:7:3: TD002 Missing author in TODO; try: `# TODO(): ...` - | -7 | # TODO: this has no author -8 | # FIXME: neither does this -9 | # TODO : and neither does this - | ^^^^ TD002 - | + | + 7 | # TODO: this has no author + 8 | # FIXME: neither does this + 9 | # TODO : and neither does this + | ^^^^ TD002 +10 | # foo # TODO: this doesn't either + | + +TD002.py:8:9: TD002 Missing author in TODO; try: `# TODO(): ...` + | + 8 | # FIXME: neither does this + 9 | # TODO : and neither does this +10 | # foo # TODO: this doesn't either + | ^^^^ TD002 + | diff --git a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-colon_TD004.py.snap b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-colon_TD004.py.snap index d470269debd43..136cc3e46c881 100644 --- a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-colon_TD004.py.snap +++ b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-colon_TD004.py.snap @@ -18,14 +18,34 @@ TD004.py:5:3: TD004 Missing colon in TODO 7 | # TODO(evanrittenhouse 😀) this has no colon | ^^^^ TD004 8 | # FIXME add a colon +9 | # foo # TODO add a colon | TD004.py:6:3: TD004 Missing colon in TODO - | -6 | # TODO this has no colon -7 | # TODO(evanrittenhouse 😀) this has no colon -8 | # FIXME add a colon - | ^^^^^ TD004 - | + | + 6 | # TODO this has no colon + 7 | # TODO(evanrittenhouse 😀) this has no colon + 8 | # FIXME add a colon + | ^^^^^ TD004 + 9 | # foo # TODO add a colon +10 | # TODO this has a colon but it doesn't terminate the tag, so this should throw. https://www.google.com + | + +TD004.py:7:9: TD004 Missing colon in TODO + | + 7 | # TODO(evanrittenhouse 😀) this has no colon + 8 | # FIXME add a colon + 9 | # foo # TODO add a colon + | ^^^^ TD004 +10 | # TODO this has a colon but it doesn't terminate the tag, so this should throw. https://www.google.com + | + +TD004.py:8:3: TD004 Missing colon in TODO + | + 8 | # FIXME add a colon + 9 | # foo # TODO add a colon +10 | # TODO this has a colon but it doesn't terminate the tag, so this should throw. https://www.google.com + | ^^^^ TD004 + | diff --git a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-description_TD005.py.snap b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-description_TD005.py.snap index c6bd1eb87a765..42d4b11d99703 100644 --- a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-description_TD005.py.snap +++ b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-description_TD005.py.snap @@ -18,6 +18,7 @@ TD005.py:5:3: TD005 Missing issue description after `TODO` 7 | # TODO(evanrittenhouse) | ^^^^ TD005 8 | # FIXME +9 | # foo # TODO | TD005.py:6:3: TD005 Missing issue description after `TODO` @@ -26,6 +27,15 @@ TD005.py:6:3: TD005 Missing issue description after `TODO` 7 | # TODO(evanrittenhouse) 8 | # FIXME | ^^^^^ TD005 +9 | # foo # TODO + | + +TD005.py:7:9: TD005 Missing issue description after `TODO` + | +7 | # TODO(evanrittenhouse) +8 | # FIXME +9 | # foo # TODO + | ^^^^ TD005 | diff --git a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-link_TD003.py.snap b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-link_TD003.py.snap index 421a9f9c0143f..4558235455146 100644 --- a/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-link_TD003.py.snap +++ b/crates/ruff/src/rules/flake8_todos/snapshots/ruff__rules__flake8_todos__tests__missing-todo-link_TD003.py.snap @@ -29,11 +29,21 @@ TD003.py:25:3: TD003 Missing issue link on the line following this TODO 29 | # TDO-3870 | -TD003.py:29:3: TD003 Missing issue link on the line following this TODO +TD003.py:29:9: TD003 Missing issue link on the line following this TODO | 29 | # TDO-3870 30 | -31 | # TODO: here's a TODO on the last line with no link +31 | # foo # TODO: no link! + | ^^^^ TD003 +32 | +33 | # TODO: here's a TODO on the last line with no link + | + +TD003.py:31:3: TD003 Missing issue link on the line following this TODO + | +31 | # foo # TODO: no link! +32 | +33 | # TODO: here's a TODO on the last line with no link | ^^^^ TD003 |