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

Add linter for commented R code #83

Merged
merged 1 commit into from
Apr 13, 2015
Merged

Conversation

jackwasey
Copy link

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

res <- re_matches(source_file$file_lines,
rex("#", except("'"), anything,
or(some_of("{}[]"), # code-like parentheses
"<-", "->", "==", # an assignment
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