Skip to content
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

First draft #1

Merged
merged 38 commits into from
Apr 12, 2021
Merged

First draft #1

merged 38 commits into from
Apr 12, 2021

Conversation

lorenzwalthert
Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Apr 9, 2021

Basically re-implements pat-s/styler in a better way.

  • re-export full styler API like for maximum ease of use and compatibility. Exposed API is hence 1:1 that of {styler}, but style guide defaults to mlr_style everywhere.
  • lean by design: instead of complete fork, only host style guide and tests, not styling infrastructure. Lower maintenance, no merge conflicts on fork updates, less convolution etc. Since we must access styler infra functions, this package will always have an R CMD check note on the use of :::.
  • use a different package name so people can use the tidyverse style from {styler} for some projects alongside with mlr style easily. Previously, installing {styler} from CRAN would 'overwrite' the mlr fork.
  • <- now replaced with = outside function calls (improves Don't replace <- with = within function call pat-s/styler#1, which essentially stopped styling <- because it could be used in function calls).
  • more tests.
  • README
    I am currently working on showcasing and documenting how third-party style guides should be distributed in Document how to create custom style guides and distribute them in a vignette r-lib/styler#721.

This improves also the end user experience:

TODO:

  • look at the diff of mlr3 when this style guide is applied and provide feedback on which changes are undesired. We can try to adapt the rules for these cases.
  • depricate old repo
  • change instructions in mlr wiki, communicate change for users.
  • look at commented transformer functions that were added to styler since initial implementation (currently commented out in mlr_style, along some that were removed on purpose because tidyverse != mlr style). Which ones should we add too? Like line break after ggplot2 + etc.

@lorenzwalthert lorenzwalthert marked this pull request as draft April 9, 2021 11:29
…ot since style could not be specified and transformer also not
…ot since style could not be specified and transformer also not
@lorenzwalthert lorenzwalthert marked this pull request as ready for review April 9, 2021 12:36
@lorenzwalthert lorenzwalthert requested a review from pat-s April 10, 2021 09:01
@pat-s
Copy link
Member

pat-s commented Apr 12, 2021

Oh wow, that's lots of work - thanks @lorenzwalthert!

Should we merge this as a first workable version and address the ToDO's in separate issues?
These might be addressable more easily then with dedicated, smaller PRs.

What do you think?

@lorenzwalthert
Copy link
Collaborator Author

Should we merge this as a first workable version and address the ToDO's in separate issues?
These might be addressable more easily then with dedicated, smaller PRs.

Sure. Then, I'd expect you to open issues for the things you find 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants