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

Braces in function calls #428

Open
krlmlr opened this Issue Sep 28, 2018 · 7 comments

Comments

Projects
None yet
2 participants
@krlmlr
Member

krlmlr commented Sep 28, 2018

Can we keep the newline after ( here?

text <- '
eventReactive(
  {
    code()
  },
  {
    code()
  }
)
'

styler::style_text(text)
#> 
#> eventReactive({
#>   code()
#> }, {
#>   code()
#> })

Created on 2018-09-28 by the reprex package (v0.2.1)

@lorenzwalthert

This comment has been minimized.

Collaborator

lorenzwalthert commented Sep 28, 2018

Maybe. I liked it that way more though, it's more compact. Is there a rule in the tidyverse style guide that discourages this use? Note that with strict = FALSE, the output is not as desired, i.e. there should be indention for the braces:

text <- '
eventReactive({

    code()
},
    {
    code()
    }
    )
    '

styler::style_text(text, strict = FALSE)
#> 
#> eventReactive({
#> 
#>   code()
#> },    {
#>   code()
#> }
#> )

Created on 2018-09-28 by the reprex package (v0.2.1)

@krlmlr

This comment has been minimized.

Member

krlmlr commented Sep 29, 2018

It's more compact, but it's much less readable, because it's very easy to overlook the }, { . And when you find it, you wonder what the meaning is.

I'm not aware of any specific rule.

@lorenzwalthert

This comment has been minimized.

Collaborator

lorenzwalthert commented Sep 30, 2018

So then if you prefer another behavior, I suggest to propose/formalize a rule in tidyverse/style first :-). In the meantime, we nevertheless need to fix the brace indention with strict = FALSE, so I opened #430.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Oct 1, 2018

We only have a contradicting rule in 2.4:

Code that is related to a brace (e.g., an if clause, a function declaration, a trailing comma, …) must be on the same line as the opening brace.

The two braces are not related, one could argue that they shouldn't be on the same line by negation.

If we add additional arguments, indentation starts looking really weird, although no rule seems to specify this either:

text <- '
eventReactive(
  {
    code()
  },
  {
    code()
  },
  label = "label"
)
'

styler::style_text(text)
#> 
#> eventReactive({
#>   code()
#> }, {
#>   code()
#> },
#> label = "label"
#> )

Created on 2018-10-01 by the reprex package (v0.2.1.9000)

The style guide seems to prefer brevity over comprehensiveness. If we want a comprehensive style guide, we could add an appendix where all the rules are formalized. Or we just use common sense and agreement :-o

@krlmlr

This comment has been minimized.

Member

krlmlr commented Oct 4, 2018

Also interesting:

text <- '
library(dplyr, warn.conflicts = FALSE)
tibble(x = 1) %>% 
  mutate(y = { x <- 2; x }, z = x)
'

styler::style_text(text)
#> 
#> library(dplyr, warn.conflicts = FALSE)
#> tibble(x = 1) %>%
#>   mutate(y = {
#>     x <- 2
#>     x
#>   }, z = x)

Created on 2018-10-04 by the reprex package (v0.2.1.9000)

I think that function arguments that consist of a braced expression always need to start on a new line, unless it's the last argument and all other arguments fit on the line of the function call:

tryCatch(
  {
  },
  error = function(e) {
  }
)

with_envvar(c(ENV = "var"), {
})

test_that("test", {
})
@krlmlr

This comment has been minimized.

Member

krlmlr commented Oct 14, 2018

Working on that Shiny code now, letting styler have its way even if I don't like the formatting because I'm tired of undoing... (And we called it "non-invasive" ;-) )

Any chance we can achieve consensus here?

@lorenzwalthert

This comment has been minimized.

Collaborator

lorenzwalthert commented Oct 14, 2018

Note that we pretty much had this discussion before: #125. Also, removing the rule remove_line_break_before_curly_opening() at first glance seems to result in the behavior you desire. However, the problem is that with that, we can't get a typical edge case right:

test_that("xyz", {
  call()
})

I.e. in the above case, we'd move the opening { to the next line too. I think originally, for this reason, I decided to go for the more compact representation because it was simpler to implement.

Because we'd need to set line break based on multi-line attribute, which is not easily possible currently, as outlined in #357.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment