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

False positive open_curly_linter / pipe_continuation_linter in magrittr pipelines #1028

Closed
AshesITR opened this issue Mar 29, 2022 · 6 comments · Fixed by #1116
Closed

False positive open_curly_linter / pipe_continuation_linter in magrittr pipelines #1028

AshesITR opened this issue Mar 29, 2022 · 6 comments · Fixed by #1116

Comments

@AshesITR
Copy link
Collaborator

There is no way to make this piece of code lint-free without rewriting:

letters %>%
{ # - open_curly_linter
  tibble(
    lo = .,
    hi = toupper(.)
  )
} %>%
  mutate(row_id = row_number())

letters %>% { # - pipe_continuation_linter
  tibble(
    lo = .,
    hi = toupper(.)
  )
} %>%
  mutate(row_id = row_number())
@MichaelChirico
Copy link
Collaborator

Slight clarification: the trailing comment is not part of the issue:

letters %>%
{
  tibble(
    lo = .,
    hi = toupper(.)
  )
} %>%
  mutate(row_id = row_number())

also lints.

We should probably prefer to align to what styler does in this case (though it may be hard to predict if styler will change behavior in the future unless the action is tied to a specific style rule):

styler::style_text("
letters %>%
{
  tibble(
    lo = .,
    hi = toupper(.)
  )
} %>%
  mutate(row_id = row_number())
")

gives

letters %>%
  {
    tibble(
      lo = .,
      hi = toupper(.)
    )
  } %>%
  mutate(row_id = row_number())

Starting from the second option gives the same:

styler::style_text("
letters %>% {
  tibble(
    lo = .,
    hi = toupper(.)
  )
} %>%
  mutate(row_id = row_number())
")
letters %>%
  {
    tibble(
      lo = .,
      hi = toupper(.)
    )
  } %>%
  mutate(row_id = row_number())

@MichaelChirico
Copy link
Collaborator

Very similar to #487 (maybe the same?)

@AshesITR
Copy link
Collaborator Author

AshesITR commented Apr 5, 2022

Worth considering separately for the to-be brace_linter.

@bersbersbers
Copy link

Worth considering separately for the to-be brace_linter.

This is still an issue with brace_linter as of 36a18e0, see also

lintr/R/brace_linter.R

Lines 53 to 54 in afcbdb3

"Opening curly braces should never go on their own line and",
"should always be followed by a new line."

@AshesITR
Copy link
Collaborator Author

Thanks for reminding me, we should add an exclusion if the preceding token is a magrittr pipe.
AFAIK base pipe doesn't support this syntax so no need to include that.

@bersbersbers
Copy link

we should add an exclusion if the preceding token is a magrittr pipe.

Adding a comma and an opening parenthesis to that exclusion would also fix #487 if I am not mistaken (but I may be!).

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 a pull request may close this issue.

3 participants