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.

Sign up for GitHub

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

Convert todo_comment_linter to XPath approach #2438

Merged
merged 12 commits into from
Dec 15, 2023
Merged

Convert todo_comment_linter to XPath approach #2438

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d56e7f4
split out todo
MichaelChirico Dec 14, 2023
b56d0a3
split out commented_code
MichaelChirico Dec 14, 2023
96ac1ed
Merge commit 'd56e7f46e1bcbceee35f1c5cd322ebd548604e7e' into todo-split
MichaelChirico Dec 14, 2023
dc8c29e
remove todo_comment_linter
MichaelChirico Dec 14, 2023
1fd852e
remove commented_code_linter
MichaelChirico Dec 14, 2023
5aaf8a1
restore
MichaelChirico Dec 14, 2023
3b3fd36
switch to XPath approach
MichaelChirico Dec 14, 2023
6007d4a
Merge remote-tracking branch 'origin/main' into todo-xml
MichaelChirico Dec 14, 2023
80e4c7c
restore
MichaelChirico Dec 14, 2023
11cd69c
expression level
MichaelChirico Dec 15, 2023
003814e
Merge branch 'main' into todo-xml
MichaelChirico Dec 15, 2023
2da039b
expression level xml
MichaelChirico Dec 15, 2023
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
31 changes: 13 additions & 18 deletions R/todo_comment_linter.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#'
#' Check that the source contains no TODO comments (case-insensitive).
#'
#' @param todo Vector of strings that identify TODO comments.
#' @param todo Vector of case-insensitive strings that identify TODO comments.
#'
#' @examples
#' # will produce lints
Expand Down Expand Up @@ -42,23 +42,18 @@
#' @export
todo_comment_linter <- function(todo = c("todo", "fixme")) {
todo_comment_regex <- rex(one_or_more("#"), any_spaces, or(todo))
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
Linter(function(source_expression) {
tokens <- with_id(source_expression, ids_with_token(source_expression, "COMMENT"))
are_todo <- re_matches(tokens[["text"]], todo_comment_regex, ignore.case = TRUE)
tokens <- tokens[are_todo, ]
lapply(
split(tokens, seq_len(nrow(tokens))),
function(token) {
Lint(
filename = source_expression[["filename"]],
line_number = token[["line1"]],
column_number = token[["col1"]],
type = "style",
message = "Remove TODO comments.",
line = source_expression[["lines"]][[as.character(token[["line1"]])]],
ranges = list(c(token[["col1"]], token[["col2"]]))
)
}

Linter(linter_level = "file", function(source_expression) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why apply this on the file-level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking there's not much benefit to being expression-level for what's basically a simple text-match linter. But there might be a ton of comments in a huge file, so maybe caching would help.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking we should specify the lowest cacheable level, which should be expression here.

I have an idea cooking on how to speed up expression-level linters by running them on the file-level if they support it and being smart about the caching. But that's not completely ripe yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds exciting. I think something like this is definitely possible and would be a big boon to anyone who never caches (like me 😅)

xml <- source_expression$full_xml_parsed_content
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved

comment_expr <- xml_find_all(xml, "//COMMENT")
are_todo <- re_matches(xml_text(comment_expr), todo_comment_regex, ignore.case = TRUE)

xml_nodes_to_lints(
comment_expr[are_todo],
source_expression = source_expression,
lint_message = "Remove TODO comments.",
type = "style"
)
})
}
2 changes: 1 addition & 1 deletion man/todo_comment_linter.Rd
View file
Open in desktop

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

Loading