New linters and bug fixes #214

fangly · 2017-03-08T09:30:55Z

No description provided.

…nportable_path_linter() with a "lax" option for fewer false positibe lints.

…_terminator_linter() * find some trailing semicolons not previously detected * find compound semicolons, i.e. semicolon between two statements on the same line * "type" option to lint "trailing" and/or "compound" semicolons

* features of the linter described at r-lib#194 * default_linters() changed accordingly (enforce lowerCamelCase) * refactored make_object_linter() since it returned too many objects that were out of the scope, and failed to return some that were

@example

* Do not lint comments within roxygen blocks (this follows the linter's description, strictly speaking). This allows to write comments that show the expected result in @example roxygen blocks, e.g.: #' @examples #' foo(1) ## list(1) * Do not consider "-" an R operator to avoid too many false positives, e.g. in "# Non-existent"

* do not display markers when check is NULL * fixed malformed error messages * more consistent errors messages * check validity of Lint fields in 'checks' * return invisible(NULL), just like expect_error() * doc update

jimhester

I think we need to deprecate the removed functions with .Deprecated() for at least one release, for the object linters like snake_case_linter I think you can just have it call object_name_linter("snake_case") internally.

More generally I think we should move away from using regular expressions if possible and towards using the token dataframe or more ideally XPath expressions on the XML versions of the parse tokens.

The regular expressions and custom logic make the function definitions pretty hard to follow and maintain, having each use a (perhaps complex) XPath expression could potentially homogenize the implementations.

jimhester · 2017-03-08T13:14:20Z

R/undesirable_function_linter.R

+  "detach"="use roxygen2's @importFrom statement in packages, or `::` in scripts",
+  "library"= "use roxygen2's @importFrom statement in packages, or `::` in scripts",
+  "require"= "use roxygen2's @importFrom statement in packages, or `::` in scripts",
+  "loadNamespace"="use `::` or requireNamespace()",


I do not think loadNamespace should be linted, I actually prefer using it in most cases, most of the time you use it for suggested packages and do want to throw an error if a package is not installed. See http://r-pkgs.had.co.nz/namespace.html#search-path for a discussion of the differences.

If you are generally aiming to follow Hadley's style and guidelines, loadNamespace should be left in by default.

jimhester · 2017-03-08T13:16:15Z

R/undesirable_function_linter.R

+  "source"=NA,
+  "sapply"="use vapply() or lapply()",
+  "mapply"="use Map()",
+  "ifelse"="use an if () {} else {} block",


While it is true some (most?) of the time people use ifelse when it would be better to use if() there are cases where you do want to branch on a vectorized conditional, I think it is too aggressive to lint this.

Ok, I'll remove it from the default. I will likely make it available a set of extended defaults for those that prefer to lint aggressively.

jimhester · 2017-03-08T13:17:30Z

R/undesirable_function_linter.R

+  "sapply"="use vapply() or lapply()",
+  "mapply"="use Map()",
+  "ifelse"="use an if () {} else {} block",
+  "return"="let the last value of a function automatically be returned",


return() is still very useful for early returns, you would need to only lint this at the end of a function body.

Sure, that just reflected my preference. I will remove if from the defaults. In the case of return, a separate linter would be preferable anyway,

jimhester · 2017-03-08T13:22:26Z

R/undesirable_function_linter.R

+  "mapply"="use Map()",
+  "ifelse"="use an if () {} else {} block",
+  "return"="let the last value of a function automatically be returned",
+  "substring"="use substr()"


Hmm I think this is also too aggressive, substring recycles its first argument, which makes it useful for some things.

substring("abcdefghijklmnop", seq(1, 15, 5)) #> [1] "abcdefghijklmnop" "fghijklmnop" "klmnop"

No worries. Again, this only reflected my own personal biases.

jimhester · 2017-03-08T13:26:36Z

R/path_linters.R

+  )
+}
+
+is_long_path <- function(str) {


I think path is a better name for this argument in all of these path functions.

The three functions is_valid_path(), is_long_path() and is_valid_long_path() do indeed expect some sort of path as input. It makes sense to change the argument name as you suggested.

I am not so convinced about changing the argument name for the four functions is_absolute_path(), is_root_path(), is_relative_path(), is_path() because the input is a string and the functions determine if this is a path.

Fine, split_path() arguments need to be path and sep though.

I thought we were splitting hairs, so I changed it to "path" anyway.
I will do the same for split_path as well.

jimhester · 2017-03-08T13:28:28Z

R/object_name_linters.R

+#' @param style UpperCamelCase, lowerCamelCase, alllowercase, ALLUPPERCASE, snake_case or
+#'              dotted.case
+#' @export
+object_name_linter <- function(style = "lowerCamelCase") {


The default needs to be snake_case.

jimhester · 2017-03-08T13:33:28Z

R/comment_linters.R

+        code_locs <<- register_code_locations(source_file, code_locs)
+      } else {
+        # non-code section: detect and report TODOs
+        if (length(file_lines) == 0L) {file_lines <- ""}  # workaround rex bug goo.gl/2rbZDy


I would rather have a full link rather than short url, as the full link contains information that can be used to retrieve the issue even if the link breaks in the future, such as repo username and issue number.

Yes, that is understandable.

jimhester · 2017-03-08T13:39:13Z

R/comment_linters.R

+#' @describeIn linters  Check that the source contains no TODO comments (case-insensitive).
+#' @param todo  Vector of strings that identify TODO comments.
+#' @export
+todo_comment_linter <- function(todo=c("todo", "fixme")) {


I feel like this function should just use the getParseData() and check for a COMMENT token that has text ^#[[:spaces:]]TODO, why is all this complication needed?

The function is more complicated than anticipated because not all comments are reported within source_file$parsed_content.

# TODO 1 x <- 3 # TODO 2 foo <- function(x) { # TODO 3 # TODO 4 x * 2 # TODO 5 } y <- foo(x) # TODO 6 # TODO 7

In the example above, only TODO 3-5 are returned as tokens. In semicolon_terminator_linter(), there is a similar issue that is addressed by the locate_other_sc function.

…fault lintr naming style

Deprecated function / replacement function / alias: * camel_case_linter / object_name_linter / - * snake_case_linter/ object_name_linter / - * multiple_dots_linter / object_name_linter / - * trailing_semicolons_linter / semicolon_terminator_linter / semicolon_terminator_linter(type = "trailing) * absolute_paths_linter / absolute_path_linter / absolute_path_linter(lax = TRUE)

Use lintr:::expect_lint() instead of expect_lint() because it is a private function

jimhester · 2017-03-09T15:06:10Z

Note: there is no alias for camel_case_linter(), snake_case_linter() and multiple_dots_linter() because they identify names to remove, while object_name_linter() identifies names to keep.

Yes this is true, we will probably need to keep the old definitions around until these functions are completely deprecated and gone then.

jimhester · 2017-03-09T15:10:42Z

R/path_linters.R

+  )
+}
+
+is_long_path <- function(str) {


Fine, split_path() arguments need to be path and sep though.

jimhester · 2017-03-09T15:14:00Z

R/path_linters.R

+  for (q in type) {
+    str <- re_substitutes(str, rex(start, q, capture(zero_or_more(any)), q, end), "\\1")
+  }
+  stringi::stri_unescape_unicode(str)


We cannot depend on stringi, you can look at base alternatives if you need this http://stackoverflow.com/a/25310392/2055486

fangly · 2017-03-10T15:51:22Z

I have implemented an unescape() function that relies on base R's gregexpr and regmatches() instead of stringi. I think there are no more open issues, Jim.

codecov-io · 2017-03-10T16:17:09Z

Codecov Report

Merging #214 into master will increase coverage by 4.33%.
The diff coverage is 96.46%.

@@            Coverage Diff             @@
##           master     #214      +/-   ##
==========================================
+ Coverage   81.31%   85.65%   +4.33%     
==========================================
  Files          30       37       +7     
  Lines        1702     2133     +431     
==========================================
+ Hits         1384     1827     +443     
+ Misses        318      306      -12

Impacted Files	Coverage Δ
R/cache.R	`95.06% <ø> (-1.24%)`	❌
R/zzz.R	`0% <0%> (ø)`	✅
R/lint.R	`71.28% <100%> (+1.02%)`	✅
R/undesirable_function_linter.R	`100% <100%> (ø)`
R/T_and_F_symbol_linter.R	`100% <100%> (ø)`
R/extraction_operator_linter.R	`100% <100%> (ø)`
R/object_name_linters.R	`100% <100%> (ø)`
R/implicit_integer_linter.R	`100% <100%> (ø)`
R/deprecated.R	`100% <100%> (ø)`
R/undesirable_operator_linter.R	`100% <100%> (ø)`
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e16d6b...ae2870d. Read the comment docs.

jimhester · 2017-03-10T22:03:25Z

Ok I think all that is left is updating the NEWS file and README explaining the changes / listing the new linters added.

fangly · 2017-03-13T12:36:11Z

This is done. It is a minor commit, so, I merging this PR now.

jimhester · 2017-03-13T12:54:51Z

Please let me merge major things like this.

fangly · 2017-03-13T13:44:46Z

Ok, sorry for my impatience. Thank you, Jim.

fangly added 14 commits March 8, 2017 10:20

Replaced the absolute_paths_linter() by absolute_path_linter() and no…

75e1f07

…nportable_path_linter() with a "lax" option for fewer false positibe lints.

New unneeded_concatenation_linter()

422c891

New T_and_F_symbol_linter()

12d3edb

New todo_comment_linter()

f6c0584

New implicit_integer_linter()

acf0383

New extraction_operator_linter()

34eec99

New undesirable_operator_linter() and undesirable_function_linter()

63d5c61

Small fix for path linter tests (commit b9727c9581)

8b80c21

Small fix for implicit integer linter tests (commit fdb37d3b53)

b9e71c0

Bugfix: Save cache in the requested directory (r-lib#213)

ece1e7d

Fixed expect_lint() issues (r-lib#211)

bd6cdb5

* do not display markers when check is NULL * fixed malformed error messages * more consistent errors messages * check validity of Lint fields in 'checks' * return invisible(NULL), just like expect_error() * doc update

jimhester reviewed Mar 8, 2017

View reviewed changes

fangly added 7 commits March 9, 2017 09:58

object_name_linter() now defaults to using style="snake_case", the de…

e3ad4f5

…fault lintr naming style

Saved in wrong folder

cf54dc6

Saved in wrong folder

bd079da

Save deprecated.R file in right folder

b46b7c7

Save test-deprecated.R file in right folder

0429802

Fix failing expect_lint() examples

3b9d77d

Use lintr:::expect_lint() instead of expect_lint() because it is a private function

fangly mentioned this pull request Mar 9, 2017

get_source_expressions() removes some tokens #215

Closed

fangly added 2 commits March 9, 2017 12:55

Renamed function

19b02ee

Use long URL

9e01b3b

jimhester reviewed Mar 9, 2017

View reviewed changes

fangly added 3 commits March 9, 2017 17:16

Adding tests for defaults

088efc9

Migrating defaults to zzz.R

ea32b26

Migrating defaults to zzz.R

f730de4

some tests for unquoting and unescaping

1abc0d7

fangly added 6 commits March 10, 2017 16:56

Update NAMESPACE

a41fba0

Updated DESCRIPTION Collate field

35ea487

Trying to fix failing examples

6134e6e

a

bd8cb86

Up-to-date Rd file for linters

4ef533d

Added Rd file for default_undesirable_functions

f4be05a

fangly added 2 commits March 12, 2017 10:27

Updated NEWS and README

a51c268

Fix data.frame.lints issue

ae2870d

fangly closed this Mar 13, 2017

jimhester reopened this Mar 13, 2017

jimhester merged commit 27a5d95 into r-lib:master Mar 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New linters and bug fixes #214

New linters and bug fixes #214

fangly commented Mar 8, 2017

jimhester left a comment

jimhester Mar 8, 2017

fangly Mar 9, 2017

jimhester Mar 8, 2017

fangly Mar 9, 2017

jimhester Mar 8, 2017

fangly Mar 9, 2017

jimhester Mar 8, 2017

fangly Mar 9, 2017

jimhester Mar 8, 2017

fangly Mar 9, 2017 •

edited

Loading

jimhester Mar 9, 2017

fangly Mar 9, 2017

jimhester Mar 8, 2017

jimhester Mar 8, 2017

fangly Mar 9, 2017

jimhester Mar 8, 2017

fangly Mar 9, 2017

jimhester commented Mar 9, 2017

jimhester Mar 9, 2017

jimhester Mar 9, 2017

fangly commented Mar 10, 2017 •

edited

Loading

codecov-io commented Mar 10, 2017 •

edited

Loading

jimhester commented Mar 10, 2017

fangly commented Mar 13, 2017

jimhester commented Mar 13, 2017

fangly commented Mar 13, 2017

New linters and bug fixes #214

New linters and bug fixes #214

Conversation

fangly commented Mar 8, 2017

jimhester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fangly Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimhester commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fangly commented Mar 10, 2017 • edited Loading

codecov-io commented Mar 10, 2017 • edited Loading

Codecov Report

jimhester commented Mar 10, 2017

fangly commented Mar 13, 2017

jimhester commented Mar 13, 2017

fangly commented Mar 13, 2017

fangly Mar 9, 2017 •

edited

Loading

fangly commented Mar 10, 2017 •

edited

Loading

codecov-io commented Mar 10, 2017 •

edited

Loading