Skip to content

Conversation

@jackwasey
Copy link

This linter looks for code-like comments, allowing roxygen blocks which may contain examples.

Copy link
Member

Choose a reason for hiding this comment

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

Should this check for = assignment rather than == equality?

I assume you restricted the search to assignments only because it could be valid to have a comment like

# checks that var > 2
if (var > 2) {
  x
}

@jackwasey
Copy link
Author

Yes, I did mean it to be equality, although my comment says otherwise. I thought too many people would legitimately use an equals symbol in a text comment. Overall, this linter is going to have trouble with false positives or false negatives, and I thought it better to err on the side of not giving spurious messages. I think it would be nice to have this, even if it only catches 75%, but prompts people to think about commented code, which, AFAIK is considered poor form.

@jimhester
Copy link
Member

I agree with you and think it is wise to restrict this to only a few operators rather than all of them.

Alternatively one way to go would be to include all of the operators in your regex and parse (but not execute) the code in the comment, then only generate a lint if the code parses without error.

@jimhester
Copy link
Member

The looping can be made a little simpler by the fact that you don't need to search globally on the line, (since once you start a comment the rest of the line is commented) and just using an lapply loop. i.e.

commented_code_linter <- function(source_file) {
  res <- re_matches(source_file$file_lines,
                    rex("#", except("'"), anything,
                        or(some_of("{}[]"), # code-like parentheses
                           "<-", "->", "==", # an assignment
                           group(graphs, "(", anything, ")"), # a function call
                           group("!", alphas) # a negation
                        )
                    ),
                    global = FALSE, locations = TRUE)
  line_numbers <- rownames(na.omit(res))
  lapply(line_numbers, function(line_number) {
    Lint(
      filename = source_file$filename,
      line_number = line_number,
      column_number = res[line_number, "start"],
      type = "style",
      message = "Commented code should be removed.",
      line = source_file$file_lines[line_number],
      linter = "commented_code_linter"
    )
  })
}

Which passes all of the same tests

@jackwasey
Copy link
Author

I like it. Executing a parse on the comments would be more sensitive without losing much or any specificity, but I don't think I have time to learn how to do that. I assume you mean this would be in addition to the regex above.

@jimhester jimhester merged commit 6d10a42 into r-lib:master Apr 13, 2015
@jimhester
Copy link
Member

Yes exactly, it didn't really take too much code to do in the end.

see 0fb0871 if you are interested in the details, I changed some things around and added support for all the operators, but the bulk of it is your original pull. The only thing I think would be nice is to detect multi-line code comments (as they won't be parse-able split into individual lines). But it is more a nice to have rather than essential, so I think it is great as is.

Thanks again for the pull request!

@jackwasey
Copy link
Author

That's fantastic. Now I'll see if my lint is still clean with your version!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants