-
Notifications
You must be signed in to change notification settings - Fork 70
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
Inconsistency w.r.t. vertical whitespace #875
Comments
Thanks @MichaelChirico.
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 PS: For the future, I'd prefer the following procedure:
Is that ok for you? |
That makes sense, and I did file tidyverse/style#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 |
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.
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
|
I agree with "when in doubt, keep as is" -- I had in mind cases where the style guide is not explicit, but 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. |
Currently
styler
appears inconsistent w.r.t. vertical whitespace.Compare:
PS the tidyverse guide is currently silent on any rules here, so inconsistency is the only per se issue with
styler
. I have filed tidyverse/style#185 to try and nudge the guide to be more explicit here.The text was updated successfully, but these errors were encountered: