-
Notifications
You must be signed in to change notification settings - Fork 186
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
fix delint conflicts #687
fix delint conflicts #687
Conversation
@@ -1,6 +1,6 @@ | |||
linters: with_defaults( # The following TODOs are part of an effort to have {lintr} lint-free (#584) | |||
line_length_linter = line_length_linter(120), | |||
cyclocomp_linter = cyclocomp_linter(29), # TODO reduce to 15 | |||
cyclocomp_linter = cyclocomp_linter(30), # TODO reduce to 15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#673 added 1 to the cyclocomp of lint()
.
To keep this separate from removing ~ 200 lints from tests/, I temporarily bump the limit here.
@@ -60,11 +60,6 @@ test_that("returns the correct linting", { | |||
expected_message <- tryCatch(parse(text = "\\"), error = get_base_message) | |||
expect_lint("\\", rex(expected_message)) | |||
|
|||
# also try when LANGUAGE initially unset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes non-english locales to fail the test.
test-exclusions.R was not touched since it contains 3 calls (and will be split by #660)
@MichaelChirico can you re-approve? I had a merge conflict and test failure on non-english locale due to |
That |
Ah, the problem was that in hindsight, removing the update of |
OK, we'll add that back in the future |
Unfortunately, some newly enabled linters had hits in
tests
, so we aren't lint-free on master atm.