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

Why is the indent different in functions without braces? #535

Closed
krlmlr opened this issue Aug 5, 2019 · 5 comments · Fixed by #536

Comments

@krlmlr
Copy link
Member

commented Aug 5, 2019

I'm using a hammer h() to turn off R CMD check warnings for functions that use NSE. Is there an easy way to make the bad (and then perhaps also the use_case) example behave?

styler::style_text("
good <- function() {
  h(
    code
  )
}
bad <- function() h(
  code
)
use_case <- function() h(~ {
  code
})
")
#> 
#> good <- function() {
#>   h(
#>     code
#>   )
#> }
#> bad <- function() h(
#>     code
#>   )
#> use_case <- function() h(~ {
#>     code
#>   })

Created on 2019-08-05 by the reprex package (v0.3.0)

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2019

Seems like the indention of <- and h( is adding up when h is within a function. Must change how to compute the indention. But I assume these are not tidyverse style guide compliant anyways because multi-line function declarations require braces? At least if-else, return and similar multi-line statements require it. I am not sure I get why you need the use case, but you could also

use_case <- rlang::as_function(h(~ {
  code
}))

Which would give the correct formatting be more tidyverse style guide compliant I guess.

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2019

I found the problem, the function keyword causes indention if the body of the function is not wrapped in curly braces. We could additionally check with styler:::needs_indention_one() if there is another token on the same line that's going to cause indention and hence, we should deactivate it for function. I think this is complicating things and we better just wrap the body into curly braces, so it is consistent with similar situations we solved that way, that is multi-line expressions for the following cases:

  • if (else)
  • while
  • for
  • return

Otherwise, one might also argue that

if (cond) h(
  something
)

is good practice, which I think is not encouraged by the tidyverse style guide. Altough for other topics (#428), I was prefering compactness, I think here it actually making things much less readable. For functions affecting the control flow, it was argued that they should have their own blocks (tidyverse/style#99), which I think also makes sense for function. What do you think?

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2019

It seems though that for strict = FALSE, we should anyways solve the case for

bad <- function() h(
  code
)

to remain unchanged with styling to be consistent with the other rules.

@krlmlr

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

Agree to fix for strict = FALSE only. I can solve using a custom function constructor indeed, it doesn't seem to be that urgent.

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2019

I just want to solve it now :-). Both cases. I also fixed some related cases in #536.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.