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

Force brace expression in function calls be on their own line #543

Merged

Conversation

@lorenzwalthert
Copy link
Collaborator

commented Sep 17, 2019

The main motivation is to avoid tryCatch() expressions to be re-formatted compactly:

styler::style_text("tryCatch(
  {
    x
  }, error = function(e) e
)")
#> tryCatch({
#>   x
#> }, error = function(e) e)

Created on 2019-09-17 by the reprex package (v0.3.0)

While at the same time, preserve formatting of testthat() expressions:

styler::style_text("testthat(x, {
  code(to(exec))
})")
#> testthat(x, {
#>   code(to(exec))
#> })

Created on 2019-09-17 by the reprex package (v0.3.0)

The abstract rule was formulated in #428 (comment).

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 or they are named.

The last part of the sentence or they are named. is needed for this case:

testthat(x, 
  b = 
  {
    5
})

Where we should rearrange to

testthat(x, 
  b = {
  5
})

Closes #428.

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 or they are named.
@lorenzwalthert lorenzwalthert force-pushed the lorenzwalthert:fix-braces-in-function-calls branch from 34526e0 to 64b8689 Sep 17, 2019
@lorenzwalthert lorenzwalthert requested a review from krlmlr Sep 17, 2019
lorenzwalthert and others added 3 commits Sep 17, 2019
will merge conflict with 7ed140e
Copy link
Member

left a comment

A big leap forward, thanks a lot!

I have added more test cases in 0c844fa, are they covered already?

call(
{
1
}, a + b,

This comment has been minimized.

Copy link
@krlmlr

krlmlr Sep 18, 2019

Member

I think the a + b needs to go on a separate line. Is this related to this pull request at all?

Rationale: Multiline expressions in function calls don't contain code from other arguments.

This may be missing from the style guide, but seems obvious to me: If an argument is so complex that it needs multiple lines, we don't want it interspersed with other arguments.

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Sep 19, 2019

Author Collaborator

In the above example, I agree. However, when we have

call(
  {
    x + y
  }, {
    f(x)
    call(x = 2, f = max(n))
  }
)

I'd leave it as is opposed to changing it into

call(
  {
    x + y
  }, 
  {
    f(x)
    call(x = 2, f = max(n))
  }
)

Because indention helps seeing what belongs together and }, { for me just reads as a separator of two expressions (i.e. I don't read them as two braces anymore). What do you think.

This comment has been minimized.

Copy link
@krlmlr

krlmlr Sep 19, 2019

Member

Can we enforce the second option in strict mode but leave the first in permissive mode?

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Sep 19, 2019

Author Collaborator

I'd leave the first case as is, but I would not change the second into the first. So if your code has }, \n{, styler should not change it to }, {, and vice versa. I think that's a fair compromise 😄 . Whenever there is brace + comma + not a brace, I agree to break lines in any case. The strict argument is already too convoluted I think. If your suggestion gets accepted in tidyverse/style, I am happy to also implement it for strict = TRUE, but I think it's a detail anyways.

This comment has been minimized.

Copy link
@krlmlr

krlmlr Sep 19, 2019

Member

Works for me. Thanks for looking into it!

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Sep 22, 2019

Author Collaborator

Ok no this is more complicated to implement. I'll stick to your rule.

- Add more test cases (#13).
@codecov-io

This comment has been minimized.

Copy link

commented Sep 19, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@542fce2). Click here to learn what that means.
The diff coverage is 96%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #543   +/-   ##
=========================================
  Coverage          ?   90.83%           
=========================================
  Files             ?       43           
  Lines             ?     1801           
  Branches          ?        0           
=========================================
  Hits              ?     1636           
  Misses            ?      165           
  Partials          ?        0
Impacted Files Coverage Δ
R/io.R 84.21% <0%> (ø)
R/style-guides.R 100% <100%> (ø)
R/rules-line-break.R 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 542fce2...1c99706. Read the comment docs.

@lorenzwalthert lorenzwalthert merged commit 9d8470b into r-lib:master Sep 22, 2019
4 checks passed
4 checks passed
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lorenzwalthert lorenzwalthert deleted the lorenzwalthert:fix-braces-in-function-calls branch Sep 22, 2019
Copy link
Member

left a comment

Amazing! Looking forward to using it on my code!

})

call(
"test", {

This comment has been minimized.

Copy link
@krlmlr

krlmlr Sep 22, 2019

Member

So the newline isn't added here because it's the last argument? Can we easily check if the brace is on the same line as the opening parenthesis of the function call?

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Sep 22, 2019

Author Collaborator

Not easily but we can in eaac448d22ec000d99030a367852c5bae9b6ceb5 (pushed to master). :-)

1
},
a + b,
{

This comment has been minimized.

Copy link
@krlmlr

krlmlr Sep 22, 2019

Member

I love this! But why is the newline added here and not in the example above?

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