r-lib / styler

Non-invasive pretty printing of R code
https://styler.r-lib.org
Other
713 stars 70 forks source link

Double indentation should be an opt-in/out rule #1157

Closed mcanouil closed 9 months ago

mcanouil commented 9 months ago

Styler "recently" enforces a rule that double indent function declaration. Let's say you have the following.

f <- function(
  x,
  y
) {
  "content"
}

It will be "messed up" (sorry, personal opinion on double indentation) as:

f <- function(
    x,
    y) {
  "content"
}

I know there is #1137 open for few months now, but regardless of this PR, I do believe double indent rule should be as other rules an opt-in/out rule.

Thanks for considering this.

lorenzwalthert commented 9 months ago

Hi @mcanouil. {styler} implements the tidyverse style guide and your code does not correspond to that style. As you can read in various issues, I am not the biggest fan of double indent, but there is another way

function(x = 1, 
         y = 2) {
  NULL
}

These are the two officially supported styles and styler adheres to those.

mcanouil commented 9 months ago

I know that's a "new" tidyverse style guide rule, but that is not the point.

My question is why is it not controlled by a rule/transformer that can be opt-out? https://styler.r-lib.org/articles/remove_rules.html

EDIT: the rule/transformer regarding function declaration all uses is_double_indent_function_declaration(), seems pretty simple to offer a parameter to disable this or not, as it is possible to use a 12 spaces indentation.

For now, I was basically trashing the whole set of transformers:

styler::style_text(
  text = "
    f <- function(
    x,
    y
  ) {
    NULL
  }",
  style = function(...) {
    transformers <- styler::tidyverse_style(...)
    transformers$indention$update_indention_ref_fun_dec <- NULL
    transformers$indention$unindent_fun_dec <- NULL
    transformers$line_break$remove_line_breaks_in_fun_dec <- NULL
    transformers
  }
)
lorenzwalthert commented 9 months ago

I am happy to review and merge a PR that refactors existing rules such that the removal of the double indention is possible in an easier way, e.g. by simply remove the rule, but I won't contribute this change.

lorenzwalthert commented 9 months ago

The answer to why it is not already implemented like this is because probably I did not care or it was more complex.

mcanouil commented 9 months ago

I see. I leave up to you to close or not this issue. Thanks for the quick reply.

lorenzwalthert commented 9 months ago

Sure. Thanks @mcanouil for understanding that catering to all user requests here is difficult and my time is limited since no one pays me to work on {styler}.

mcanouil commented 9 months ago

my time is limited since no one pays me to work on {styler}.

I know this too well. That's the usual thing in packages development 😅

Anyway, thanks for the time spent on the package and replying. 😉