-
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
assignment_linter() gains operator= argument to allow, e.g. '=' assignments #2711
base: main
Are you sure you want to change the base?
Conversation
Thanks @IndrajeetPatil! I'd also like to get feedback from @AshesITR in this case since he commented a lot on the earlier PRs & this will be released soon afterwards. |
Yeah, I think it'll be good to remove it from the defaults. |
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.
Good stuff, just a few kinks that could be ironed out.
#' @evalRd rd_tags("assignment_linter") | ||
#' @seealso | ||
#' - [linters] for a complete list of linters available in lintr. | ||
#' - <https://style.tidyverse.org/syntax.html#assignment-1> | ||
#' - <https://style.tidyverse.org/pipes.html#assignment-2> | ||
#' @export | ||
assignment_linter <- function(allow_cascading_assign = TRUE, | ||
assignment_linter <- function(operator = c("<-", "<<-"), |
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.
Positional changes could affect downstream dependencies if the linter is configured using positional arguments. We should at least check for is.logical(operator)
to stop with a helpful error message.
), | ||
length(op_text) | ||
) | ||
op_lint_message_fmt[op_text %in% c("<<-", "->>")] <- |
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 path will produce a confusing message for operator = c("<-", "<<-")
when encountering "->>"
.
op_lint_message_fmt[op_text %in% c("<<-", "->>")] <- | ||
"Replace %s by assigning to a specific environment (with assign() or <-) to avoid hard-to-predict behavior." | ||
op_lint_message_fmt[op_text == "%<>%"] <- | ||
"Avoid the assignment pipe %s; prefer using <- and %%>%% separately." |
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 will be inconsistent advice if operator = "="
.
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.
For the test structure, maybe check for deprecation warnings once (maybe with smell tests that the automatic inference of operator
works correctly) and proceed testing only the new spec with all (old) tests?
That should make full removal easier down the line.
Supersedes #2698 and #2521. h/t again @J-Moravec for getting the ball rolling here.
Closes #2441.
This also un-couples the linted operator from the
allow_trailing
behavior; lints generated by the operator used are now independent of lints generated by line-trailing assignment operators:Having
<<-
allowed by default is back-compatible, but does strike me as odd -- should we also plan to remove this from the defaults?