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

Use () after all function calls #232

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Nov 15, 2020

in README.

@lionel-
Copy link
Member

lionel- commented Nov 16, 2020

Stefan prefers without parentheses.

@smbache
Copy link
Member

smbache commented Nov 16, 2020

Stefan prefers without parentheses.

Yeah, IMO that's way cleaner and less typing. Also, I think more of RHS as functions/expressions, so like in that one does not write lapply(foo, bar()). I know the analogy does not fully extend to the situation with other args, but still I prefer thinking of RHS as not being a function call standing on its own... I like the cleanliness and the interpretation that comes with no parens.. but that's just my opinion.

@krlmlr
Copy link
Member Author

krlmlr commented Nov 16, 2020

Thanks. A few thoughts:

  • Typing speed is often less relevant than reading speed, in particular for production code that is ever read after being written (e.g., any script not named "Untitled[0-9]*").
  • Always adding parentheses is consistent -- there is no difference between 1 and > 1 arguments. IMO this wins over perceived cleanliness.
  • The tidyverse style guide explicitly demands parentheses in this case.
  • I do not think of the f(...) in x %>% f(...) as a function call, with or without arguments. It could be interpreted as a special form of currying. I don't think it should be necessary to understand currying in order to understand the pipe.
  • Avoiding the special case of unary function simplifies the documentation.

How about a new section about omitting the parentheses for unary functions?

@smbache
Copy link
Member

smbache commented Nov 16, 2020

Well the style guide is obviously not a very stylish guide in this case ;-) as I said, my opinion is not the only one, and I don't have to make all decisions. I don't see omission of () any less consistent than omission of parens in arithmetic when they're not necessary and when precedence is clear.

@krlmlr
Copy link
Member Author

krlmlr commented Dec 5, 2020

#236 is another reason to teach users the longer form first:

library(magrittr)
mtcars$cyl %>%
  forcats::as_factor
#> Error in .::forcats: unused argument (as_factor)

Created on 2020-12-05 by the reprex package (v0.3.0)

What's a good way to move forward?

@luisDVA
Copy link

luisDVA commented Jan 18, 2022

Hi all,
For teaching purposes, I'm trying to find any information or history on why/how %>% allows for function names without parenthesis on the RHS (for single argument uses). I tried to figure out if internally the pipe is using match.fun or something else but I am still unsure. I think this is relevant, given that the new base pipe does not allow functions on the RHS without parenthesis even when the only argument is whatever is being piped into them.

Thanks!

@IndrajeetPatil
Copy link

FWIW, not including a function call produces a lint:

library(lintr)

# will produce lints
lint(
  text = "1:3 %>% mean %>% as.character",
  linters = pipe_call_linter()
)
#> <text>:1:9: warning: [pipe_call_linter] Use explicit calls in magrittr pipes, i.e., `a %>% foo` should be `a %>% foo()`.
#> 1:3 %>% mean %>% as.character
#>         ^~~~
#> <text>:1:18: warning: [pipe_call_linter] Use explicit calls in magrittr pipes, i.e., `a %>% foo` should be `a %>% foo()`.
#> 1:3 %>% mean %>% as.character
#>                  ^~~~~~~~~~~~

# okay
lint(
  text = "1:3 %>% mean() %>% as.character()",
  linters = pipe_call_linter()
)

Created on 2022-10-11 with reprex v2.0.2

Additionally, native pipe requires a function call:

library(lintr)
lint(
  text = "1:3 |> mean |> as.character",
  linters = pipe_call_linter()
)
#> <text>:1:1: error: [error] The pipe operator requires a function call as RHS
#> 
#> ^

Created on 2022-10-11 with reprex v2.0.2

For these reasons, I agree with Kirill here that the README should include function calls.

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 this pull request may close these issues.

None yet

5 participants