Fix #260 #262
Fix #260 #262
Conversation
Codecov Report
@@ Coverage Diff @@
## master #262 +/- ##
==========================================
- Coverage 86.12% 85.88% -0.24%
==========================================
Files 39 40 +1
Lines 2183 2225 +42
==========================================
+ Hits 1880 1911 +31
- Misses 303 314 +11
Continue to review full report at Codecov.
|
|
Sorry about the missing coverage report. I manually made sure all of |
|
You can ignore the coverage report issues. |
| types <- factor(vapply(object, `[[`, character(1), "type"), | ||
| levels = c("style", "warning", "error")) | ||
| tbl <- table(filenames, types) | ||
| filenames <- rownames(tbl) |
There was a problem hiding this comment.
change this line to
filenames <- rownames(tbl) %||% character()Then you can remove the entire conditional at https://github.com/jimhester/lintr/pull/262/files#diff-a0a9d3c78e9f113319e5e069fb2697ecR155
tests/testthat/checkstyle.xml
Outdated
| @@ -1,5 +1,5 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <checkstyle version="lintr-1.0.0.9001"> | |||
| <checkstyle version="lintr-1.0.1"> | |||
There was a problem hiding this comment.
Try to keep unrelated changes out of PRs. If you do include them they should be in separate commits.
There was a problem hiding this comment.
Sorry about that. And given that you just bumped the version, I think I also created a merge conflict. Resolving in a separate commit.
Removes extra conditional.
tests/testthat/test-methods.R
Outdated
| on.exit(unlink(file)) | ||
|
|
||
| writeLines(good, con = file, sep = "\n") | ||
| expect_silent(out <- summary(lint(file))) |
There was a problem hiding this comment.
I think this test would be more clear if you did something like
no_lints <- lint(
"if (1 > 0){
print(\"good\")
}")
no_lint_summary <- summary(no_lints)
# ... tests
has_lints <- lint(
"if(1>0){
print ('bad')
}")
has_lint_summary <- summary(has_lints)
# ... testsAlso I would explicitly pass linters to lint() rather than relying on the default ones. The defaults can change over time so they could potentially break this test if new linters are added.
There was a problem hiding this comment.
Good call. Just cleaned this up.
- Make the intent of the `summary.lints()` tests clearer. - Divide into 2 tests (no lints vs lints found). - Use assignment linter explicitly.
|
Ok, all this needs is a note in the NEWS.md file explaining the new feature and thanking yourself and this PR. |
|
Done, thanks! |
|
Thanks! |
summary.lints()function. Taken from @jimhester in Attenuate the verbosity in lint_package() #260 and modified. I have a mild preference tosummary.lints()on tidy and untidy code.tests/testthat/checkstyle.xml(version given must agree withlintrversion).test-methods.R.