tidyverse / style

The tidyverse style guide for R code
https://style.tidyverse.org
Other
298 stars 104 forks source link

Tweak long argument advice #174

Closed hadley closed 3 years ago

lorenzwalthert commented 3 years ago

This addresses https://github.com/tidyverse/style/issues/173 (and https://github.com/r-lib/styler/issues/829) I think. I must say I am quite surprised to see this proposed by @hadley. I can't really follow the rationale of introducing double indention to the style guide, there is no such thing so far AFAIK in this style guide.

~I think it's quite a fundamental change, requiring adaption for {styler}, {lintr} as well as IDEs such as RStudio or vs code language server to give the expected indention on ENTER. Is this already set in stone? As one can probably guess, I am not really in favor of that change 😄 , but it's obviously not up to me to decide on it...~

Ok maybe I am just too used to the old style and worked too many hours to implement this in {styler} to have a helpful opinion on that... haha. Adaption requirments for IDEs and {styler} and {lintr} are still relevant though.

hadley commented 3 years ago

I opened this to kick off internal discussion.

lorenzwalthert commented 3 years ago

Ok, thanks for explaining.

luciorq commented 3 years ago

The definition of each argument in its line is also more suitable for tracking changes in version control.

It is indeed about time to have it addressed somewhere in the R ecosystem.

But, what is the rationale for double indent? There is any language or style guide that already uses it? I haven't stumbled upon it before.

hadley commented 3 years ago

Now that we've discussed it internally and I've made some changes to my original proposal, it's now worth looking at @lorenzwalthert. The double indentation is inspired by Google's style guide for C++.

hadley commented 3 years ago

https://github.com/rstudio/rstudio/pull/9766 adds IDE support

hadley commented 3 years ago

Would love to get your feedback on this before Sept 3 (one week) so we can make a decision on whether or not to move forward.

MichaelChirico commented 3 years ago

LGTM cc @michaelquinn32

michaelquinn32 commented 3 years ago

LGTM. Thanks to everyone that worked on this.

We've got some really gnarly long names lurking in our codebase, and I am very happy to say goodbye the ugly hacks we've had to implement to get them formatted correctly. I think this also goes a long way towards addressing some other internal concerns, like how long names with parentheses-aligned arguments produce a lot of whitespace between the function name and the body of the code.

lorenzwalthert commented 3 years ago

Thanks for adding context (Google C++ style guide), it makes more sense now. I am ok with this, but I don't think it's necessary to always use double indention. If you can make all lines less than 80 characters with the existing rule, I think this is fine:

# all lines less than 80 characters
long_function_name <- function(a = "a long argument",
                               b = "another argument",
                               c = "another long argument") {
  # As usual code is indented by two spaces.
  ...
}

As I understand, the primary case you want to tackle is this:

# some lines longer than 80 characters
long_function_name_that_makes_at_least_one_arg_over_80_characters <- function(a = "a long argument",
                                                                              b = "another argument", 
                                                                              c = "another long argument") {
  # As usual code is indented by two spaces.
  ...
}

Then, proposed re-arrangement makes most sense.

long_function_name_that_makes_at_least_one_arg_over_80_characters <- function(
    a = "a long argument",
    b = "another argument", 
    c = "another long argument") {
  # As usual code is indented by two spaces.
  ...
}
hadley commented 3 years ago

@lorenzwalthert I don't consider the style guide to be binding; it's just a suggestion. I think it's better to keep the guide as short and prescriptive as possible, recognising that anyone is free to deviate it from it at any time for any reason.

lorenzwalthert commented 3 years ago

Ok. I see that we can’t specify all cases here and that it is for the reader’s convenience we keep it brief. In general, I consider this style guide to be binding for {styler} tough, so I plan to implement whatever we decide in this issue for the sake of consistency (even if I did not agree with everything 😊).

hadley commented 3 years ago

@lorenzwalthert in that case, I think it's better to be consistent and always indent function args by four spaces — it's a simpler rule, thus easier to remember. (Assuming that they don't all fit on one line)

hadley commented 3 years ago

@lorenzwalthert you and @lionel- have convinced me that I was being too restrictive, and I've reworded to allow either function-indent or double-indent style (preferring function-indent when it fits).

To make it easier to switch between the different types, @lionel- is going to work on an add-in that can easily toggle between the different styles, and if you're interested, he'll add it to styler.

lorenzwalthert commented 3 years ago

Ok, yes, in principle interested, we have to see how we can handle this in {styler}, what the default should be and how it plays with IDEs like RStudio.