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

No blank lines in function headers #630

Merged
merged 5 commits into from Apr 11, 2020

Conversation

lorenzwalthert
Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Apr 11, 2020

Closes #549.

@lorenzwalthert lorenzwalthert merged commit 58f852d into r-lib:master Apr 11, 2020
@lorenzwalthert lorenzwalthert deleted the issue-549 branch April 11, 2020 11:42
@krlmlr
Copy link
Member

krlmlr commented Apr 12, 2020

This might be too strict. Noticed in cynkra/dm@155570b.

With this PR

styler::style_text("c(\n  a,\n\n  b\n)")
#> c(
#>   a,
#>   b
#> )

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

CRAN version

styler::style_text("c(\n  a,\n\n  b\n)")
#> c(
#>   a,
#> 
#>   b
#> )

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

@lorenzwalthert
Copy link
Collaborator Author

lorenzwalthert commented Apr 12, 2020

This behavior was introduced in #629. I think it’s exactly as intended. When and why would you prefer the blank line not to be removed? F you Walt to visually separate lines, you can put a comment on the blank line

@krlmlr
Copy link
Member

krlmlr commented Apr 12, 2020

The empty line allows for better structuring of the code in my case, see cynkra/dm@155570b. I'm really looking for

Screenshot from 2020-04-12 08-10-40

@krlmlr
Copy link
Member

krlmlr commented Apr 12, 2020

Adding an empty comment as a placeholder works but adds noise. Can we be more liberal with empty lines, e.g. only remove if strict = TRUE?

@lorenzwalthert
Copy link
Collaborator Author

lorenzwalthert commented Apr 12, 2020

It is only for strict = FALSE:

styler::style_text(
  "c(
  1, 
  
  2
  )
  ",
  strict = FALSE
)
#> c(
#>   1,
#> 
#>   2
#> )

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

The change introduced with this PR concerns function headers:

function(x, 
     
         y) {
  # comment 
}

But I wonder if there are other ways to accommodate for your use case. Can you specify conditions under which you want to allow blank lines, e.g.

  • function calls with more than k lines.
  • arguments must be named
  • arguments must be multi-line.
  • only for functions like ifelse(), switch() and friends.
    etc.?

@krlmlr
Copy link
Member

krlmlr commented Apr 12, 2020

Indeed, missed that. I also like how the empty line is kept with strict = FALSE at the beginning of function code.

styler::style_text('
f <- function(a = 1,
              b = 2) {

  here("we should keep", "that empty line above")
}', strict = FALSE)
#> 
#> f <- function(a = 1,
#>               b = 2) {
#> 
#>   here("we should keep", "that empty line above")
#> }

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

Maybe if there's a comment, we should keep all empty lines around that comment?

@lorenzwalthert
Copy link
Collaborator Author

yeah but empty lines after function header is another topic, let's discuss that in #371. Also, there is no intention to remove this empty line, rather in #371 we may even add one for longer function declarations.

Can you specify conditions under which you want to allow blank lines, e.g.

  • function calls with more than k lines.
  • arguments must be named
  • arguments must be multi-line.
  • only for functions like ifelse(), switch() and friends.
    ?

Any insights on that from your side? While it is ok to defer to strict = FALSE, this also has a lot of other implications one does not want because I think strict is overloaded.

@krlmlr
Copy link
Member

krlmlr commented Apr 12, 2020

Maybe if there's a comment, we should keep all empty lines around that comment?

@lorenzwalthert
Copy link
Collaborator Author

lorenzwalthert commented Apr 12, 2020

You mean to preserve the blank lines in

utils::globalVariables(c(
   # pipe
   ".",
  
   # nesting
   "data",
   ...
))

for example?

This sounds reasonable. Do you think there are cases when one has a blank line in his or her code and it's unintentional? Because if that does not hold, we should revert this PR. An alternative would be to extend the API and let strict be:

  • a boolean
  • a named list of booleans, each referring to an aspect that can be treated strictly or not, e.g. blank lines in function calls, spacing around operators such as + etc.

I think implementing this would require not so much effort and could be of great benefit for people who want to use strict = TRUE for most but not all cases ruled by strict currently.

@krlmlr
Copy link
Member

krlmlr commented Apr 12, 2020

I'd rather discuss strict elsewhere. I think #549 ultimately was about function headers, this PR also changes formatting of long function calls. Can we disentangle?

If in doubt, user code is correct and should be kept unchanged, also with strict = TRUE.

My huge switch() in cynkra/dm@155570b should be rewritten, let's not bother here.

@lorenzwalthert
Copy link
Collaborator Author

I'd rather discuss strict elsewhere. I think #549 ultimately was about function headers, this PR also changes formatting of long function calls. Can we disentangle?

Well the problem was I forgot to update one test in the other PR and then I rebased after merge but for some reason the commit was still part of this PR so sorry for the convolution. I think function headers are not a concern so much and after resolving #630 we can focus on function calls in #632.

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.

Redundant line breaks in function header not removed when formatting
2 participants