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

Implement flake8_fixme and refactor TodoDirective #4681

Merged
merged 15 commits into from
Jun 2, 2023
Merged
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
8 changes: 8 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_fixme/T00.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# TODO: todo
# todo: todo
# XXX: xxx
# xxx: xxx
# HACK: hack
# hack: hack
# FIXME: fixme
# fixme: fixme
30 changes: 27 additions & 3 deletions crates/ruff/src/checkers/tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
use rustpython_parser::lexer::LexResult;
use rustpython_parser::Tok;

use crate::directives::TodoComment;
use crate::lex::docstring_detection::StateMachine;
use crate::registry::{AsRule, Rule};
use crate::rules::ruff::rules::Context;
use crate::rules::{
eradicate, flake8_commas, flake8_implicit_str_concat, flake8_pyi, flake8_quotes, flake8_todos,
pycodestyle, pylint, pyupgrade, ruff,
eradicate, flake8_commas, flake8_fixme, flake8_implicit_str_concat, flake8_pyi, flake8_quotes,
flake8_todos, pycodestyle, pylint, pyupgrade, ruff,
};
use crate::settings::Settings;
use ruff_diagnostics::Diagnostic;
Expand Down Expand Up @@ -60,6 +61,8 @@ pub(crate) fn check_tokens(
]);
let enforce_extraneous_parenthesis = settings.rules.enabled(Rule::ExtraneousParentheses);
let enforce_type_comment_in_stub = settings.rules.enabled(Rule::TypeCommentInStub);

// Combine flake8_todos and flake8_fixme so that we can reuse detected [`TodoDirective`]s.
let enforce_todos = settings.rules.any_enabled(&[
Rule::InvalidTodoTag,
Rule::MissingTodoAuthor,
Expand All @@ -68,6 +71,10 @@ pub(crate) fn check_tokens(
Rule::MissingTodoDescription,
Rule::InvalidTodoCapitalization,
Rule::MissingSpaceAfterTodoColon,
Rule::LineContainsFixme,
Rule::LineContainsXxx,
Rule::LineContainsTodo,
Rule::LineContainsHack,
]);

// RUF001, RUF002, RUF003
Expand Down Expand Up @@ -184,9 +191,26 @@ pub(crate) fn check_tokens(
}

// TD001, TD002, TD003, TD004, TD005, TD006, TD007
// T001, T002, T003, T004
if enforce_todos {
let todo_comments: Vec<TodoComment> = indexer
.comment_ranges()
.iter()
.enumerate()
.filter_map(|(i, comment_range)| {
let comment = locator.slice(*comment_range);
TodoComment::from_comment(comment, *comment_range, i)
})
.collect();

diagnostics.extend(
flake8_todos::rules::todos(&todo_comments, indexer, locator, settings)
.into_iter()
.filter(|diagnostic| settings.rules.enabled(diagnostic.kind.rule())),
);

diagnostics.extend(
flake8_todos::rules::todos(indexer, locator, settings)
flake8_fixme::rules::todos(&todo_comments)
.into_iter()
.filter(|diagnostic| settings.rules.enabled(diagnostic.kind.rule())),
);
Expand Down
6 changes: 6 additions & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,12 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
// airflow
(Airflow, "001") => (RuleGroup::Unspecified, Rule::AirflowVariableNameTaskIdMismatch),

// flake8-fixme
(Flake8Fixme, "001") => (RuleGroup::Unspecified, Rule::LineContainsFixme),
(Flake8Fixme, "002") => (RuleGroup::Unspecified, Rule::LineContainsTodo),
(Flake8Fixme, "003") => (RuleGroup::Unspecified, Rule::LineContainsXxx),
(Flake8Fixme, "004") => (RuleGroup::Unspecified, Rule::LineContainsHack),

_ => return None,
})
}
193 changes: 191 additions & 2 deletions crates/ruff/src/directives.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
//! Extract `# noqa` and `# isort: skip` directives from tokenized source.
//! Extract `# noqa`, `# isort: skip`, and `# TODO` directives from tokenized source.

use std::str::FromStr;

use bitflags::bitflags;
use ruff_text_size::{TextLen, TextRange, TextSize};
Expand Down Expand Up @@ -217,6 +219,133 @@ fn extract_isort_directives(lxr: &[LexResult], locator: &Locator) -> IsortDirect
}
}

/// A comment that contains a [`TodoDirective`]
pub(crate) struct TodoComment<'a> {
/// The comment's text
pub(crate) content: &'a str,
/// The directive found within the comment.
pub(crate) directive: TodoDirective<'a>,
/// The comment's actual [`TextRange`].
pub(crate) range: TextRange,
/// The comment range's position in [`Indexer`].comment_ranges()
pub(crate) range_index: usize,
}

impl<'a> TodoComment<'a> {
/// Attempt to transform a normal comment into a [`TodoComment`].
pub(crate) fn from_comment(
content: &'a str,
range: TextRange,
range_index: usize,
) -> Option<Self> {
TodoDirective::from_comment(content, range).map(|directive| Self {
content,
directive,
range,
range_index,
})
}
}

#[derive(Debug, PartialEq)]
pub(crate) struct TodoDirective<'a> {
/// The actual directive
pub(crate) content: &'a str,
/// The directive's [`TextRange`] in the file.
pub(crate) range: TextRange,
/// The directive's kind: HACK, XXX, FIXME, or TODO.
pub(crate) kind: TodoDirectiveKind,
}

impl<'a> TodoDirective<'a> {
/// Extract a [`TodoDirective`] from a comment.
pub(crate) fn from_comment(comment: &'a str, comment_range: TextRange) -> Option<Self> {
// The directive's offset from the start of the comment.
let mut relative_offset = TextSize::new(0);
let mut subset_opt = Some(comment);

// Loop over `#`-delimited sections of the comment to check for directives. This will
// correctly handle cases like `# foo # TODO`.
while let Some(subset) = subset_opt {
let trimmed = subset.trim_start_matches('#').trim_start();

let offset = subset.text_len() - trimmed.text_len();
relative_offset += offset;

// If we detect a TodoDirectiveKind variant substring in the comment, construct and
// return the appropriate TodoDirective
if let Ok(directive_kind) = trimmed.parse::<TodoDirectiveKind>() {
let len = directive_kind.len();

return Some(Self {
content: &comment[TextRange::at(relative_offset, len)],
range: TextRange::at(comment_range.start() + relative_offset, len),
kind: directive_kind,
});
}

// Shrink the subset to check for the next phrase starting with "#".
subset_opt = if let Some(new_offset) = trimmed.find('#') {
relative_offset += TextSize::try_from(new_offset).unwrap();
subset.get(relative_offset.to_usize()..)
} else {
None
};
}

None
}
}

#[derive(Debug, PartialEq)]
pub(crate) enum TodoDirectiveKind {
Todo,
Fixme,
Xxx,
Hack,
}

impl FromStr for TodoDirectiveKind {
type Err = ();

fn from_str(s: &str) -> Result<Self, Self::Err> {
// The lengths of the respective variant strings: TODO, FIXME, HACK, XXX
for length in [3, 4, 5] {
let Some(substr) = s.get(..length) else {
break;
};

match substr.to_lowercase().as_str() {
"fixme" => {
return Ok(TodoDirectiveKind::Fixme);
}
"hack" => {
return Ok(TodoDirectiveKind::Hack);
}
"todo" => {
return Ok(TodoDirectiveKind::Todo);
}
"xxx" => {
return Ok(TodoDirectiveKind::Xxx);
}
_ => continue,
}
}

Err(())
}
}

impl TodoDirectiveKind {
fn len(&self) -> TextSize {
match self {
TodoDirectiveKind::Xxx => TextSize::new(3),
TodoDirectiveKind::Hack | TodoDirectiveKind::Todo => TextSize::new(4),
TodoDirectiveKind::Fixme => TextSize::new(5),
}
}
}

#[cfg(test)]
mod tests {
use ruff_text_size::{TextLen, TextRange, TextSize};
Expand All @@ -225,7 +354,9 @@ mod tests {

use ruff_python_ast::source_code::{Indexer, Locator};

use crate::directives::{extract_isort_directives, extract_noqa_line_for};
use crate::directives::{
extract_isort_directives, extract_noqa_line_for, TodoDirective, TodoDirectiveKind,
};
use crate::noqa::NoqaMapping;

fn noqa_mappings(contents: &str) -> NoqaMapping {
Expand Down Expand Up @@ -428,4 +559,62 @@ z = x + 1";
vec![TextSize::from(13)]
);
}

#[test]
fn todo_directives() {
let test_comment = "# TODO: todo tag";
let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len());
let expected = TodoDirective {
content: "TODO",
range: TextRange::new(TextSize::new(2), TextSize::new(6)),
kind: TodoDirectiveKind::Todo,
};
assert_eq!(
expected,
TodoDirective::from_comment(test_comment, test_comment_range).unwrap()
);

let test_comment = "#TODO: todo tag";
let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len());
let expected = TodoDirective {
content: "TODO",
range: TextRange::new(TextSize::new(1), TextSize::new(5)),
kind: TodoDirectiveKind::Todo,
};
assert_eq!(
expected,
TodoDirective::from_comment(test_comment, test_comment_range).unwrap()
);

let test_comment = "# fixme: fixme tag";
let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len());
let expected = TodoDirective {
content: "fixme",
range: TextRange::new(TextSize::new(2), TextSize::new(7)),
kind: TodoDirectiveKind::Fixme,
};
assert_eq!(
expected,
TodoDirective::from_comment(test_comment, test_comment_range).unwrap()
);

let test_comment = "# noqa # TODO: todo";
let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len());
let expected = TodoDirective {
content: "TODO",
range: TextRange::new(TextSize::new(9), TextSize::new(13)),
kind: TodoDirectiveKind::Todo,
};
assert_eq!(
expected,
TodoDirective::from_comment(test_comment, test_comment_range).unwrap()
);

let test_comment = "# no directive";
let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len());
assert_eq!(
None,
TodoDirective::from_comment(test_comment, test_comment_range)
);
}
}
14 changes: 13 additions & 1 deletion crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,11 @@ ruff_macros::register_rules!(
rules::flake8_todos::rules::MissingSpaceAfterTodoColon,
// airflow
rules::airflow::rules::AirflowVariableNameTaskIdMismatch,
// flake8-fixme
rules::flake8_fixme::rules::LineContainsTodo,
rules::flake8_fixme::rules::LineContainsHack,
rules::flake8_fixme::rules::LineContainsXxx,
rules::flake8_fixme::rules::LineContainsFixme,
);

pub trait AsRule {
Expand Down Expand Up @@ -821,6 +826,9 @@ pub enum Linter {
/// [flake8-todos](https://github.com/orsinium-labs/flake8-todos/)
#[prefix = "TD"]
Flake8Todos,
/// [flake8-fixme](https://github.com/tommilligan/flake8-fixme)
#[prefix = "T"]
Flake8Fixme,
/// [eradicate](https://pypi.org/project/eradicate/)
#[prefix = "ERA"]
Eradicate,
Expand Down Expand Up @@ -955,7 +963,11 @@ impl Rule {
| Rule::MissingTodoColon
| Rule::MissingTodoDescription
| Rule::InvalidTodoCapitalization
| Rule::MissingSpaceAfterTodoColon => LintSource::Tokens,
| Rule::MissingSpaceAfterTodoColon
| Rule::LineContainsFixme
| Rule::LineContainsHack
| Rule::LineContainsTodo
| Rule::LineContainsXxx => LintSource::Tokens,
Rule::IOError => LintSource::Io,
Rule::UnsortedImports | Rule::MissingRequiredImport => LintSource::Imports,
Rule::ImplicitNamespacePackage | Rule::InvalidModuleName => LintSource::Filesystem,
Expand Down
27 changes: 27 additions & 0 deletions crates/ruff/src/rules/flake8_fixme/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
pub mod rules;

#[cfg(test)]
mod tests {
use std::path::Path;

use anyhow::Result;
use test_case::test_case;

use crate::registry::Rule;
use crate::test::test_path;
use crate::{assert_messages, settings};

#[test_case(Rule::LineContainsFixme; "T001")]
#[test_case(Rule::LineContainsHack; "T002")]
#[test_case(Rule::LineContainsTodo; "T003")]
#[test_case(Rule::LineContainsXxx; "T004")]
fn rules(rule_code: Rule) -> Result<()> {
let snapshot = format!("{}_T00.py", rule_code.as_ref());
let diagnostics = test_path(
Path::new("flake8_fixme/T00.py"),
&settings::Settings::for_rule(rule_code),
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}
}
Loading