Inconsistency w.r.t. vertical whitespace

[MichaelChirico]

Currently styler appears inconsistent w.r.t. vertical whitespace.

Compare:

# leading/trailing whitespace is removed
styler::style_text('
function(x) {

  return(x)

}
')
# function(x) {
#   return(x)
# }

# no change
styler::style_text('
foo <- function(x) {

}
# foo <- function(x) {
# 
# }
')

# leading whitespace is unchanged
styler::style_text('
foo <- function(a) {

  # comment
  return(a)
}
# foo <- function(a) {
# 
#   # comment
#   return(a)
# }
')

PS the tidyverse guide is currently silent on any rules here, so inconsistency is the only per se issue with styler. I have filed https://github.com/tidyverse/style/issues/185 to try and nudge the guide to be more explicit here.

[lorenzwalthert]

Thanks @MichaelChirico.

• For case 1: One blank line after the function formals should probably tolerated, maybe depending on the body length, see https://github.com/r-lib/styler/issues/371.

• For case 2: For empty function declarations, I indeed agree that formatting could be improved:

function(x) {



}

For case 3: Probably an artifact going back to more than 5 years. I can't remember on top of my head why there is a check for not a comment in set_line_break_around_curly(), probably we wanted to make sure that if the same are on the same line, like { #, we don't add a line break, but that has the side effect of also not removing some.

---

PS: For the future, I'd prefer the following procedure:

• For major styling rules, {styler} will only support formatting that is explicitly encouraged in the tidyverse style guide. Please file an issue in the style repo and later here if the changes are accepted.
• As in my understanding, the style guide should be concise and can't handle every edge case. So some case won't be covered explicitly in the style guide. If you find such behavior, you can post it here directly.

Is that ok for you?

[MichaelChirico]

That makes sense, and I did file https://github.com/tidyverse/style/issues/185 simultaneously with this.

I filed this instead of just waiting for upstream because of the apparent inconsistency.

Conversely, would you also say it's a bug if styler is making changes that aren't mentioned in the style guide?

[lorenzwalthert]

Conversely, would you also say it's a bug if styler is making changes that aren't mentioned in the style guide?

It depends. Because the style guide is kept concise, I don’t think Hadley wrote it with the idea in mind that it should be a complete specification for {styler} (or {lintr}). Do you know how this is handled in other languages?

Also, {styler} is non-invasive. While formatting spacing, indention and tokens are clearly specified in the style guide, I think for line breaks, there should be generally more flexibility with a bias towards if in doubt, keep as if. Consequentially, I think if spaces, indention and line breaks are not formatted according to the style guide, they are bugs. For line breaks, I don’t think so necessarily.

I filed this instead of just waiting for upstream because of the apparent inconsistency.

For me, this issue is rather a small inconsistency compared to others. I am willing to fix case 2 from above, but the other cases I don’t think we should change anything. Case 3 is also in line with a rule we have for blank lines in function calls I realize. see NEWS.md:

blank lines in function calls and headers are now removed, for the former only when there are no comments before or after the blank line (#629, #630, #635, #723).

[MichaelChirico]

I agree with "when in doubt, keep as is" -- I had in mind cases where the style guide is not explicit, but styler nevertheless makes changes.

The example from the NEWS is one such case -- as noted, the style guide doesn't say anything about vertical whitespace. Wouldn't the "light touch" approach then be to leave those blank lines alone?

[lorenzwalthert]

I had in mind cases where the style guide is not explicit, but styler nevertheless makes changes.

The example from the NEWS is one such case -- as noted, the style guide doesn't say anything about vertical whitespace. Wouldn't the "light touch" approach then be to leave those blank lines alone?

That's one approach. Or to try to get these rules formalised in the style guide. A few rules emerged in {styler} and were later submitted in the style repo. Or we say it's a detail and decide on handling them in {styler} directly until someone raises an issue in the style repo and can push a rule through that is different from the current handling.