Skip to content

{ / } and line breaks in function calls #125

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

Closed
lorenzwalthert opened this issue Aug 14, 2017 · 9 comments
Closed

{ / } and line breaks in function calls #125

lorenzwalthert opened this issue Aug 14, 2017 · 9 comments

Comments

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Aug 14, 2017

The tidyverse style guide states in section 2.4 that

A } should always go on its own line, unless it’s followed by else or ).

So that's why we implemented remove_line_break_before_round_closing() which is responsible for
the line breaks removed in tikzDevice.

I personally think that's fine, but we may only include this rule if strict = TRUE.

Also it says that a { should never go on its own line and should always be followed by a new line, so

tryCatch(
{
  ...
} 

from the above-referenced commit is in my eyes not compliant with the tidyverse style guide.

@lorenzwalthert lorenzwalthert changed the title keep line break between } and )? { / } and line breaks Aug 14, 2017
@krlmlr
Copy link
Member

krlmlr commented Aug 14, 2017

The rules might need a refinement. In this example, the closing brace } belongs to an argument that is indented already, which is why indenting the closing parenthesis looks wrong:

# good
fun(arg, {
  block
})

fun(
  arg,
  {
    block
  }
)

fun(
  {
    block
  },
  arg
)

# bad

fun({
  block
},
{
  another block
})

fun(
  arg,
  {
    another block
  })

@lorenzwalthert
Copy link
Collaborator Author

lorenzwalthert commented Aug 14, 2017

I am not sure whether I understand you correctly. Why is

fun(
  arg,
  {
    block
  }
)

a good example with regard to the second rule cited? Shouldn't it be

fun(
  arg, {
    block
  }
)

or even

fun(
  arg, {
    block
})

Also

fun(
  arg,
  {
    another block
  })

is bad, I agree, but I thought

fun(
  arg, {
    another block
})

would be correct, so } is unindented since a ) follows.

@krlmlr
Copy link
Member

krlmlr commented Aug 14, 2017

I haven't found a specification for long function calls in the tidyverse guide. Maybe we could contribute one, that also covers complex function arguments that consists of whole blocks?

No need to hurry though, I'm fine with this limitation for now.

krlmlr added a commit that referenced this issue Aug 17, 2017
- Solve eq_sub indention in general (#125).
@krlmlr
Copy link
Member

krlmlr commented Aug 17, 2017

Attempt to formulate a line-break rule:

A multi-line argument to a function must start on a new line, unless it's the last argument (like test_that(..., {}) and all other arguments fit on one line.

A function call must either fit on one line, or have a line break after the opening ( with the arguments indented. Exceptions are switch() and ifelse()/if_else(), and calls where the only multi-line argument is the last argument.

Example: #125 (comment)

# good
switch(var,
  case1 = ,
  case2 = "val",
  "default_val"
)

switch(var,
  case1 = {
  },
  case2 = {
  },
  {
  }
)

if_else(cond,
  true_val,
  false_val,
  na_val
)

regular_call(
  usually,
  all,
  arguments,
  go,
  on,
  a,
  separate,
  line,
  maybe,
  only,
  `if`,
  strict = TRUE
)

regular_call(all, args, fit)

# bad
regular_call(usually, all,
  arguments, ...)

regular_call(
  usually, all,
  arguments, ...)

@krlmlr
Copy link
Member

krlmlr commented Aug 17, 2017

Another potential exception, perhaps if all arguments are multi-line?

tryCatch({
  block
},
error = function(e) {
  handler
},
finally = {
  cleanup
})

Maybe not for the tidyverse style, but it looks like a valid option for other styles.

@lorenzwalthert
Copy link
Collaborator Author

I suggested a function to add a line break after the opening parenthesis if the expression is a function call, see #118 (comment). Should we implement that for the CRAN milestone or not? Otherwise,

long_call(1, 
          k * 3)

Will become just

long_call(1, 
  k * 3)

@krlmlr
Copy link
Member

krlmlr commented Aug 22, 2017

Did you mean

long_call(
  1,
  k * 3
)

?

@lorenzwalthert
Copy link
Collaborator Author

In the meanwhile, I implemented #149, which does what you wrote, yes. Otherwise, just removing the rule we had (update_indention_ref_fun_dec()) would result in what I wrote.

@krlmlr krlmlr changed the title { / } and line breaks { / } and line breaks in function calls Aug 24, 2017
krlmlr added a commit that referenced this issue Aug 24, 2017
- Vignette on customizing styler (#145).
- No line break after `switch()` and friends (#152).
- Remove flat relicts completely (#151).
- Don't reindent function calls and break line correctly for multi-line calls (#149).
- Set space between "=" and "," (#150).
- Make R CMD Check perfect (#148).
- Adding tests for exception handling with invalid parse data (#139).
- Fix indention by checking for all potential triggers (#142).
- Fix un-indention (#135).
- Support wide characters (#130).
- No spaces around :, :: and :::.
- Redesigning the API (#123).
- Solve eq_sub indention in general (#125).
- Minor refactorings.
- Re-indent token-dependent (#119).
- Supporting more indention patterns.
- Allow raw indention.
- Definitively fixing eol issue with comments.
- Infrastructure.
- Flattening out the parse table.
- New rule: no space after ! -> !!! for tidyeval.
- Fix spacing around '{'.
- Don't drop tokens! Fixes #101.
- EOL spaces in empty comments (and in general) (#98).
- mal-indention in conditional statement due to wrong specification of indent_without_paren) (#95).
- Complicated indentions based on arithmetic and special operators (#96).
- indention interaction on with assignment operator and other operators (#97).
@lorenzwalthert
Copy link
Collaborator Author

Reference: #254.

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

No branches or pull requests

2 participants