Apply 'styler' style for mlr

[pat-s]

#2278

Styler branch for mlr

See also this comment.

Styled with lorenzwalthert/styler@mlr-style by calling styler::style_pkg(style = styler::mlr_style).

Why?

Instead of investigating lintr recommendations by hand, we could apply a verified styler call to style the pkg.

Two options:

• Applied by Travis in every run (if we are absolutely sure that everything works the way we want it to)
• Applied by hand in a PR

Visible changes

• Adds { } brackets to all single-line if-statements
• adds correct spacing after #' doc comments
• uses two-spaces indentation consistently

[lorenzwalthert]

Great. Not sure about the files that were not styled because of errors. Need to investigate this. I can take a look. Also, are you happy with the visible changes or do the rules need to be adapted? I assumed you know about the scope argument, right? Changes could be applied one scope level at the time (and then be committed if ok) instead of scope = "tokens". I think strict = FALSE would not add braces to if statements, but I need to check. At least to keep alignment, it can be helpful to use strict = FALSE.

[lorenzwalthert]

Unfortunately, the branch of styler (mlr-style) you most likely based this PR on was not rebased onto upstream (r-lib/styler), so it did not contain a few patches we made since July. Apologies for that. I migrated this version to the branch lorenzwalthert/styler@mlr-style-old and then rebased on r-lib/styler@master and force-pushed the rebased version to lorenzwalthert/styler@mlr-style. So the version you used is now at lorenzwalthert/styler@mlr-style-old. With the rebased version, I don't get any errors when using default options for scope and strict for the first two problematic files you mention above (R/StackedLearner.R and R/TuneControlGenSA.R) at fdd4c7d, so I assume we could try the other ones too. One thing we could do is re-style the whole repo again on a new branch and then make a PR from styler-2 to pat-s/styler and check the diff and merge first before considering this PR (#2498) into master. I can do this if you want, assuming you have not manually tweaked any styling and used strict = TRUE and scope = "tokens", the expected changes are very limited and the diff reviewing would not take much time. I just did this and there are quite a few changes, so I assume you already reviewed manually and hence we better just style the remaining files that errored again, not the whole repo. Sorry again if you have spend a lot of time reviewing the diff for #2498 already because I have not rebased lorenzwalther/styler@mlr-style earlier.

[lorenzwalthert]

Also, I noted that the CI fails when running devtools::document() at file R/generatePartialDependence.R. I used the rebased version of styler, restyled the file locally and ran devtools::document() and did not get an error. Maybe also best to tackle this using the rebased version of styler.

[mb706]

Some thoughts:

• I support the way { } are only added to if statements if they are multi-line. One irregularity is in utils.R where a } ) (with space) is generated that should be }).
• I don't see the point in changing }) to }\n); it doesn't add anything to clarity, it completely breaks with coding style so far, and its pretty ugly
• I'd vote against breaking up things like system.time({result = longfunction()}) into multiple lines
• maybe consider changing statement_# inline comment to statement__# inline comment (two spaces) as in python
• If a function begins with an empty line, do we really want to delete that? Sometimes its nice to have some optical spacing

[lorenzwalthert]

A few notes from my side on the automation part of things:

I don't see the point in changing }) to }\n); it doesn't add anything to clarity, it completely breaks with coding style so far, and it's pretty ugly.

We should be able to revert this by re-styling with an updated style guide.

I'd vote against breaking up things like system.time({result = longfunction()}) into multiple lines

This can't currently be done with styler automatically (yet) (r-lib/styler#357).

maybe consider changing statement # inline comment to statement # inline comment (two spaces) as in python.

This should be doable too if you want to apply it to all inline comments.

If a function begins with an empty line, do we really want to delete that? Sometimes it's nice to have some optical spacing.

There were thoughts on this but nothing has been implemented yet (r-lib/styler#371). I suppose that this is a case-dependent thing we need to adjust manually.

[pat-s]

I support the way { } are only added to if statements if they are multi-line. One irregularity is in utils.R where a } ) (with space) is generated that should be }).

Hm, I would still vote in favor. It's more verbose and makes sure that nothing bad happens if for any reason the indention changes or another line is added to the if-statement.

I don't see the point in changing }) to }\n); it doesn't add anything to clarity, it completely breaks with coding style so far, and its pretty ugly

Yes, that's not so important. Can we easily adjust this in the style?

I'd vote against breaking up things like system.time({result = longfunction()}) into multiple lines

Hm, there are pros and cons. Is there something like a #no-styler tag similar to #nolint?

If a function begins with an empty line, do we really want to delete that? Sometimes its nice to have some optical spacing

I usually also do this. Would be good if we could keep this.

general

I now pushed the updated style. The two errors/warnings look like bugs of styler?
You can find them in the updated intro post of this issue.

[lorenzwalthert]

As far as not breaking lines between} and ) goes: This is implemented in the branch lorenzwalthert/styler@mlr-style:

styler::style_text(c("call({", "1})"),
  style = styler::mlr_style,
  scope = "spaces"
)
#> call({
#> 1})

^{Created on 2018-12-28 by the reprex package (v0.2.1)}

Note that this does not remove the line break if it is already there. Would you want that?

Is there something like a #no-styler tag similar to #nolint?

No, we decided not to put further energy into this (r-lib/styler#365).

If a function begins with an empty line, do we really want to delete that? Sometimes it's nice to have some optical spacing.

This is implemented currently:

styler::style_text(c("a <- function() {", "", "x", "}"),
  style = styler::mlr_style,
  scope = "spaces"
)
#> a <- function() {
#> 
#> x
#> }

^{Created on 2018-12-28 by the reprex package (v0.2.1)}

What am I missing?

For the files that throw an error, the problem is that this code can't be parsed with base R.

parse(text = "df[, get(\"meas.name\") = rank(.SD[[meas.name]], ties.method = \"average\"), by = \"task.id\"]")
#> Error in parse(text = "df[, get(\"meas.name\") = rank(.SD[[meas.name]], ties.method = \"average\"), by = \"task.id\"]"): <text>:1:23: unexpected '='
#> 1: df[, get("meas.name") =
#>                           ^

^{Created on 2018-12-28 by the reprex package (v0.2.1)}

So you can't have x[, f(a) = b] apparently, which I guess is why you've deactivated lintr for this line of code.

[pat-s]

Note that this does not remove the line break if it is already there. Would you want that?

If it is a quick thing, why not. Would make it more robust.

styler::style_text(c("a <- function() {", "", "x", "}"),
style = styler::mlr_style,
scope = "spaces"
)
#> a <- function() {
#>
#> x
#> }

Cool!

For the files that throw an error, the problem is that this code can't be parsed with base R.

Ah ok, I'll try to fix it :) Maybe we can have a "first final draft" soon.

[pat-s]

When using scope = "spaces", nested one-line if statements are falsely unindented:

• MultilabelTask.R l.18
• BaseEnsemble.R l.49
• ResampleResult_operators.R l.102
• StackedLearner.R l.244, l.623, l.726, l.730
• generateThreshVsPerf.R l.53
• selectFeaturesSequential.R l.33, l.44
• tuneFitnFun.R l.36
• tests/test_regr_randomForest.R l.87

Question: Shouldn't they already have been fixed (i.e. brackets applied) by the previous runs without scope = spaces? What is the reason they have been missed?

[lorenzwalthert]

nested one-line if statements

I am not sure what you mean. Can we work with reprex? Also, there seems to be a problem with scope = "spaces" for the deafault style guide too when the code belongs to roxygen example code. I opened r-lib/styler#454 to investigate.

Edit: I rebased the mlr-style branch on the fix closing r-lib/styler#454 (sorry I should use merge in the future), so you should now get:

styler::style_text("\n#' There\n#' @examples\nif (TRUE)\n return(x1)\nx <- y\n",
  style = styler::mlr_style, scope = "spaces"
)
#> 
#> #' There
#> #' @examples
#> if (TRUE)
#>  return(x1)
#> x <- y

^{Created on 2019-01-04 by the reprex package (v0.2.1)}

Also, we should add unit tests to the branch specifically to the mlr style so we don't destroy our work in the future.

[lorenzwalthert]

I think it's good to have a version ready only concerning spacing first and merge it asap.

[lorenzwalthert]

I also updated the styler mlr-style branch to turn }\n) into }).

styler::style_text(c("call(\n{", "1}\n)"), style = styler::mlr_style)
#> call({
#>   1
#> })

^{Created on 2019-01-04 by the reprex package (v0.2.1)}

[pat-s]

Sure, here is a reprex from BaseEnsemble.R l.49ff

(For some reason the reprex pkg renders forever for me, but here is the code)

style_text(c('  for (i in seq_along(base.learners)) {
    ps = getParamSet(base.learners[[i]])
    pids = sprintf("%s.%s", ids[i], names(ps$pars))
    for (j in seq_along(ps$pars))
      ps$pars[[j]]$id = pids[[j]]
    names(ps$pars) = pids
    par.set.bls = c(par.set.bls, ps)
  }'), 
  style = styler::mlr_style, scope = "spaces")

You'll see that ps$pars[[j]]$id = pids[[j]] is falsely unindented.

I also updated the styler mlr-style branch to turn }\n) into }).

Great!

[lorenzwalthert]

@pat-s you just found another bug in styler. Thanks. I opened r-lib/styler#456 to discuss it and resolved it in r-lib/styler#457. Then, I merged the branch into the styler mlr-style branch. It should work now:

txt <- c(
  "for (x in 1) {",
  "  x",
  "  for (x in k) ",
  "    3",
  "}"
)
styler::style_text(txt, scope = "spaces", style = styler::mlr_style)
#> for (x in 1) {
#>   x
#>   for (x in k)
#>     3
#> }

^{Created on 2019-01-05 by the reprex package (v0.2.1)}

[pat-s]

Thanks @lorenzwalthert! Looks ok now for me. I would prefer an empty line after a function initialization but if that would mean loosing all other spacing related changes, I can live with it.

What are your thoughts on this?
@mb706 @berndbischl @mllg
Just to get you up-to-date, the purpose of this PR is that calling styler::style_XXX(style = mlr_style) would style a file/pkg with the defined mlr style rules.

[lorenzwalthert]

Looks ok now for me. I would prefer an empty line after a function initialization but if that would mean loosing all other spacing related changes, I can live with it.

It's not enforced with scope = "spaces", nor are these removed. Would you like to force it with scope >= "line_breaks"? We can do it I think.

[pat-s]

It's not enforced with scope = "spaces", nor are these removed. Would you like to force it with scope >= "line_breaks"? We can do it I think.

Would be great if it is not too much work. And indeed, scope = "spaces" did not change anything.

[berndbischl]

What are your thoughts on this?
@mb706 @berndbischl @mllg

I currently dont have time, to invest into this. I am REALLY unsure I want an automatic programm iterating over code.

If you can reliably argue and demonstrate that this works, i am happy to look at this again

[lorenzwalthert]

The line break after the function declaration should now be enforced for multi-line function declarations (lorenzwalthert/styler@2a3fbc6).

[pat-s]

@lorenzwalthert In generatePartialDependenceData styler replaces all := occurences by = which causes the build to fail.

This is data.table syntax here. Any idea how to omit this behavior?

[lorenzwalthert]

Indeed. Thanks. Root: The token <- and := are both of class LEFT_ASSIGN. This is unexpected for me. I fixed the replacement by also checking the text of the token and added a unit test in lorenzwalthert/styler@8950185. It seems as if the case I was afraid of happened:

The different rules will likely interact and produce some unanticipated behavior. We saw this many times in the past.

This is the reason I am hesitating to support any other style guide officially in r-lib/styler@master unless we do a lot of unit testing.

[pat-s]

Had no time recently, will try to catch up ASAP.

[pat-s]

I think we finally have a working implementation. Thanks a ton @lorenzwalthert!

@larskotthoff Could you take a final look at the diff? Looks ok for me and the build passes as well.
Using this dedicated style created by @lorenzwalthert we can keep the code tidy and uniform using only one line of code (in my case, I mapped the command to a shortcut which I can easily apply on a file then).

[lorenzwalthert]

Glad to see that in the making. @pat-s maybe this would even be the ideal time to add unit tests to lorenzwalthert/mlr-style because we have a verified before / after comparison. Plus merging r-lib/styler into the branch would probably make sense. Since we can now select the style via Addin (see r-lib/styler#463), this would probably simplify your shortcut mapping. Note though that we currently think about changing the interface as noted in r-lib/styler#463). Also, probably best you keep a fork of my mlr-style branch also. I'd also remove the unexported function style_mlr() from the fork if you would not mind.

[jakob-r]

Can we have a rule where the newline in the function body only exists if the body has more then e.g. 10 lines.
Especially functions within an apply should not have this newline if they are quite short. Also those short S3 Functions look ugly with a new line.

[lorenzwalthert]

Fair point. I implemented it for 10 lines body. @pat-s do you want to re-style? I also merged all of r-lib/styler into the branch which most notably includes some bug-fixes plus selecting the style via Addin, so you could set the option styler.addins.style to use the mlr style and map it to a keyboard shortcut.

[pat-s]

so you could set the option styler.addins.style to use the mlr style and map it to a keyboard shortcut.

That's very convenient. Thanks.

do you want to re-style?

Yes.

I'll reply to your other comments ASAP.

@lorenzwalthert If I use the addin with the mlr-style option set, = are getting falsely replaced by <-. Does not happen when using the command from the terminal.

[jakob-r]

IMHO it looks decent now.
One could still discuss about the styling of functions like this

a = function(arg1, arg2,
  arg3, arg4
)

To be formatted like this

a = function(
  arg1, arg2,
  arg3, arg4
)

But I also kinda like that the styler is not so intrusive here.

[lorenzwalthert]

@lorenzwalthert If I use the addin with the mlr-style option set, = are getting falsely replaced by <-. Does not happen when using the command from the terminal.

This is very strange. I can't reproduce the problem. Have you really set the style? I set it via the Addin and it worked fine.

[pat-s]

This is very strange. I can't reproduce the problem. Have you really set the style? I set it via the Addin and it worked fine.

Can't reproduce it anymore.
Probably some mistake on my side :)

Glad to see that in the making. @pat-s maybe this would even be the ideal time to add unit tests to lorenzwalthert/mlr-style because we have a verified before / after comparison.

Yep, but I unfortunately have no time to do this (if this was meant as a pointer 😄 )

Plus merging r-lib/styler into the branch would probably make sense. Since we can now select the style via Addin (see r-lib/styler#463), this would probably simplify your shortcut mapping.

You already did that.

Note though that we currently think about changing the interface as noted in r-lib/styler#463).

Ok, no problem - you decide how the interface will look like. We are already happy enough with the mlr implementation.

Also, probably best you keep a fork of my mlr-style branch also. I'd also remove the unexported function style_mlr() from the fork if you would not mind.

Jep, thanks. Just forked it.

[pat-s]

Restyling a last time now and then merging 🎉

[lorenzwalthert]

Horray 🎉 well done 👍. Can I tweet about this? Which mlr repo is next? 😄 Should it be mentioned somewhere in your CONTRIBUTING.md to use the mlr style guide for styler before submitting a PR?

[pat-s]

Horray tada well done +1. Can I tweet about this?

Sure, go ahead!

Which mlr repo is next? smile Should it be mentioned somewhere in your CONTRIBUTING.md to use the mlr style guide for styler before submitting a PR?

All other repos basically, but step by step.
Yes, will include it in our coding guidelines. Have to revamp them anyhow :)

[pat-s]

@lorenzwalthert After merging latest master into my fork, I get the following error when styling

rlang::last_error()
<error>
message: argument "strict" is missing, with no default
class:   `rlang_error`
backtrace:
 1. styler:::style_active_file()
 2. styler::try_transform_as_r_file(context, transformer) R/addins.R:67:4
 3. rlang::with_handlers(...) R/addins.R:171:2
 4. base::tryCatch(...)
 5. base:::tryCatchList(expr, classes, parentenv, handlers)
 6. base:::tryCatchOne(expr, names, parentenv, handlers[[1L]])
 7. value[[3L]](cond)

I tried to debug a bit but did not get very far.
Comparing the mlr_style with the tidyverse style in styler I also do not see any difference in how argument strict is used. Can you help here?

In addition, I was wondering as if there is a better method than updating my fork every once in a while and stumbling over problems which are hard to solve for me since I am not following the immediate development of styler. Maybe we can discuss this in a separate issue in styler.

[lorenzwalthert]

Well a nice solution would be to have a travis crone job that merges the fork once a day with the upstream repo in r-lib/styler, makes sure all tests pass and then pushes this back to your repo. I will help you out and make a PR to your fork to fix things for now.

[pat-s]

Well a nice solution would be to have a travis crone job that merges the fork once a day with the upstream repo in r-lib/styler, makes sure all tests pass and then pushes this back to your repo.

Sounds like a good approach which might catch problems early on.
Is there already a setup which does that?

I will help you out and make a PR to your fork to fix things for now.

Highly appreciated! I do not want to bother you so often - that's why there needs to be a different approach to this in the future :)

[lorenzwalthert]

I don't know, you are the tic guy :-). Well, it just takes me a few minutes (<10), so never mind if you have questions. Seeing styler being used with mlr is already enough motivation for me to put in som time.

[pat-s]

I don't know, you are the tic guy :-)

Hehe yes, your saying just sounded as there was already a project which does it like that.

I'll see if I can find some time for it. Sounds like an interesting approach for sure.

Btw, how do other non-tidyverse style guide handle the "updating problem" currently? I guess there are more than just the mlr one?

[lorenzwalthert]

No there is no such project. But it might be an interesting step to use tic to set up repo mirrors in a first step and then allow for special configurations like the one discussed. There is also no other style guide (yet) that is frequently updated.