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

Other style guides #340

Closed
krlmlr opened this issue Feb 4, 2018 · 37 comments
Closed

Other style guides #340

krlmlr opened this issue Feb 4, 2018 · 37 comments

Comments

@krlmlr
Copy link
Member

krlmlr commented Feb 4, 2018

What would it take to support the following styles?

  • @yihui's, e.g. in tinytex
    • e.g., use = for assignment, use ' for strings
  • @wlandau's in drake
    • start argument list on the following line in a function declaration, no space between ) and {

I'm sure there's a related postponed issue, but I haven't looked.

Edit: This is now documented in a few vignettes and we have a GitHub template for custom style guides.

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Feb 4, 2018

I think there is no postponed issue with concrete plans on other style guides. However, I created a style guide that removes all comments in #334 (comment), and that was quick. So I think it's not a lot of work. For me, the question is also how we want to handle style guides other than the tidyverse style guide in general:

  • Should third-party style guides (contributed by anyone) be implemented in styler or distributed via an another package we maintain (e.g. named styleguides) that collects style guides or should people write their own packages when they implement style guides? We could even put the tidyverse style guide into style_guides and make styler a pure infrastructure package.
  • How can we ensure that contributed style guides are of high quality? Do we require third-party style guides to pass certain unit tests? Should the testing framework be used that was developed to unit test the tidyverse style guide? If other style guides are implemented in styler, I think we need to rethink the organisation of our unit tests.

I think these questions are important to make explicit decisions on as the number of third-party style guides may grow over time and a consistent strategy should be adopted. What's your @krlmlr take on that?

@krlmlr
Copy link
Member Author

krlmlr commented Feb 4, 2018

For these two I'd be fine if they lived in styler but were not exported for now. It feels like we need to play with a "proof of concept" first before thinking about organization. I'd let the handling of additional style guides be part of the upcoming GSoC project.

@lorenzwalthert
Copy link
Collaborator

In styler and not exported sounds good to me.

@lorenzwalthert
Copy link
Collaborator

Reference: The package oneliner implements a third-party style guide as a package and might help shaping a refined understanding of how we could organise and distribute new style guides.

@lorenzwalthert
Copy link
Collaborator

I am currently working on implementing a style guide for the mlr package within styler: mlr-org/mlr#2278

@riccardoporreca
Copy link
Contributor

riccardoporreca commented Jun 20, 2018

On the topic of supporting custom style guides, you might also want to consider the possibility of
selecting the style guide used by the RStudio addins.
I have actually an implementation of this as an additional "Set style" addin in fork miraisolutions/styler, which I have been using already to play with some style guide customization.
I did not open a dedicated issue / PR, but would be happy to do so.
Direct link to the diff.

@lorenzwalthert
Copy link
Collaborator

Thanks @riccardoporreca. I'll fork your fork (haha) and try it out when I get the chance. I first want to submit styler 1.02. to CRAN

@riccardoporreca
Copy link
Contributor

Sure, go ahead with the styler %>% fork %>% fork when you get a chance.
Happy 1.0.2 release.

@lorenzwalthert
Copy link
Collaborator

Working on the mlr style guide. Reference: mlr-org/mlr#2278

@lorenzwalthert
Copy link
Collaborator

Related: a blog post for modifying the tidyverse style guide if you are not a styler expert. https://lorenzwalthert.netlify.com/posts/customizing-styler-the-quick-way/

@Robinlovelace
Copy link

Heads-up @krlmlr I've had a bash at implementing yihui_style() in this PR: #449.

Feedback welcome!

@lorenzofanchi
Copy link

lorenzofanchi commented Apr 30, 2019

I tried to use "Set style" from the add-in menu in RStudio, and set it to

styler::tidyverse_style(scope = 'line_breaks')

This break styling, throwing the following errors:

> styler:::style_active_file()
Using style `styler::tidyverse_style(scope='line_breaks')`
Error in get_addins_style_fun()() : attempt to apply non-function
> styler:::style_selection()
Using style `styler::tidyverse_style(scope='line_breaks')`
Error in style(...) : could not find function "style"

Am I doing something wrong, or can I not customize the styling in this way?

@lorenzwalthert
Copy link
Collaborator

Not currently, but we are just working on that for a week or so (#500). Please install that branch from GitHub and tell me if it works:

remotes::install_github("r-lib/styler#500")

@lorenzofanchi
Copy link

lorenzofanchi commented Apr 30, 2019

Removed old package using remove.packages and installed new one as you suggested. Results in following error:

> library(styler)
> styler:::style_selection()
Using style transformers`styler::tidyverse_style(scope = 'line_breaks')`
Error in style(...) : could not find function "style"
> sessionInfo()
R version 3.6.0 (2019-04-26)
Platform: x86_64-apple-darwin15.6.0 (64-bit)
Running under: macOS Mojave 10.14.4

Matrix products: default
BLAS:   /System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/Versions/A/libBLAS.dylib
LAPACK: /Library/Frameworks/R.framework/Versions/3.6/Resources/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] styler_1.1.0.9000

loaded via a namespace (and not attached):
 [1] compiler_3.6.0  backports_1.1.4 magrittr_1.5    tools_3.6.0     pillar_1.3.1    rstudioapi_0.10 tibble_2.1.1    crayon_1.3.4    pkgconfig_2.0.2 rlang_0.3.4     purrr_0.3.2    

@lorenzwalthert
Copy link
Collaborator

I am not sure I can reproduce this. Can you re-start R and RStudio?

@pat-s
Copy link
Contributor

pat-s commented Apr 30, 2019

Trying to use ale with styler for VIM. There is an option to set the style guide. However, if I am not mistaken there is currently only the tidyverse style guide supported, right?

I know that you can set the style for the addin in options() but for the ALE plugin an exported style function from the styler package is required?

g:ale_r_styler_options                                 g:ale_r_styler_options                                                │ 32
                                                       b:ale_r_styler_options                                                │ 33
  Type: String                                                                                                               │ 34
  Default: 'styler::tidyverse_style'                                                                                         │ 35
                                                                                                                             │ 36
  This option can be configured to change the options for styler.                                                            │ 37
                                                                                                                             │ 38
  The value of this option will be used as the style argument for the                                                        │ 39
  styler::style_file options. Consult the styler documentation                                                               │ 40
  for more information.

I do not know how I should specify an external style function via this setting.
Maybe a similar option named styler.ale.style could be implemented?

Some of our devs are working in VIM and it would be great to use the new styler ale implementation!

(if this belongs into a separate issue, feel free to move it. Did not want to hijack that other discussion.)

@lorenzofanchi
Copy link

lorenzofanchi commented Apr 30, 2019

@lorenzwalthert
I did the following:

  1. remove package using remove.packages('styler')
  2. restart RStudio
  3. install using remotes::install_github("r-lib/styler#500")
  4. restart RStudio
  5. open file and tried to style, results as follows

Styling active document:

> styler:::style_active_file()
Using style transformers`styler::tidyverse_style()`

works

Style selection:

> styler:::style_selection()
Using style transformers`styler::tidyverse_style()`
Error in style(...) : could not find function "style"

does not work

Then I tried used "Set style" to change the style in use and set it to:

styler::tidyverse_style(scope = 'line_breaks')

Results:

  • Styling active file: works
  • Styling selection: does not work

@lorenzwalthert
Copy link
Collaborator

@lorenzofanchi sorry I missed that you were styling the selection only. Thanks for the hint. It should be fixed now in the aforementioned branch.

@lorenzofanchi
Copy link

@lorenzwalthert awesome, I'll try the updated version now!

Another question: is the set style remembered across RStudio restarts?

@lorenzwalthert
Copy link
Collaborator

No, it is not. You need to put something like this in your .Rprofile (e.g. via usethis::edit_r_profile()):

options(styler.addins_style_transformer = "styler::tidyverse_style(scope = 'line_breaks')")

So when R starts, it sets this option.

@lorenzofanchi
Copy link

Great stuff, thanks a lot for all the help and effort developing this incredibly useful tool!

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Apr 30, 2019

@pat-s: I don't use VIM, so I am not sure. Here is the PR in the ale repo: https://github.com/w0rp/ale/pull/2401/files.

From looking at the diff, I can at least tell that you'd need a function name there from some namespace. But you could use the mlr-style branch fork the fork you got from me and then specify that as the option, i.e. styler::mlr_style

If it is not working, maybe @tvatter could jump in. Also @tvatter, I think as we saw with #500, it is better to supply the transformers, not the style, because that would allow for much richer style specification. Should we do a breaking API change PR to fix this problem before the next release?

However, if I am not mistaken there is currently only the tidyverse style guide supported, right?

I don't think so. Any style guide, you just need to be able to find it at runtime. So you could also use oneliner I think. I just think it must be in a package namespace because otherwise there is no way to find it for the Rscript call (see diff of the PR linked above to the ale repo)

@tvatter
Copy link

tvatter commented Apr 30, 2019

Yeah, sorry, I didn't know much about styler and didn't really take the time to read the doc before implementing in ale. I can do a PR to ale to remove ale_r_styler_options and add ale_r_styler_style and ale_r_styler_transformers as two separate options if that provides more flexibility. In any case, using styler::mlr_style or any style defined in the namespace of styler or (in the namespace of any locally installed package) should work with the current version no?

@pat-s
Copy link
Contributor

pat-s commented May 1, 2019

From looking at the diff, I can at least tell that you'd need a function name there from some namespace. But you could use the mlr-style branch fork the fork you got from me and then specify that as the option, i.e. styler::mlr_style

I did that and it did not work. But I accidentally had CRAN styler installed for some reason. It works with the fork just as expected 👍

@tvatter
If one specifies a non-valid style in ale_r_styler_options is is just silently ignored. Some kind of matching would be great here. But the prio for this is low I would say since there are only few VIM guys that use the new feature in general and even less that use a custom style.
Thanks for all your work on this!

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented May 1, 2019

@pat-s ok great it worked. But the customization is not just for other style guides, also for customization within the tidyverse style guide. I.e. if one does want to style with scope tokens only, they could set the style transformers:

tidyverse_style(scope = "linebreaks")

To achieve that. I think this is more common.

@pat-s
Copy link
Contributor

pat-s commented May 13, 2019

What is the current policy dealing with custom style guides? Updating my fork constantly with all `styler' commits is tedious. I see that there is also #451 and possibly others?

I see the problem related to dealing with custom style guides (ensuring quality, responding to updates/PR). Maybe the easiest would be to have styler being the API with custom packages that serve specific style guides.
These would then simply "depend" on styler and only consist of e.g. mlr-style.R.
Of course loading these guides via the RStudio extension (or ale for VIM) might get a bit more complicated then.

@Robinlovelace
Copy link

Heads-up @pat-s I had a go at something similar to mlr styles here: #451

Would happily join forces on something similar - not sure what the difference between 'mlr style' and 'equal style' is though...

@tvatter
Copy link

tvatter commented Oct 16, 2019

It's good that I was mentioned in this issue, I get the reminder from time to time that I needed to address this style vs transformers in VIM. This is done now. Sorry for the delay given that there was only two lines of code to change.

@pat-s
Copy link
Contributor

pat-s commented Oct 17, 2019

Would happily join forces on something similar - not sure what the difference between 'mlr style' and 'equal style' is though...

Have no clue about your style. Major diffs of the "mlr-style" are to use 2 spaces for indentation and = instead of <-.

@Robinlovelace
Copy link

Robinlovelace commented Oct 17, 2019

Thanks for the clarification @pat-s. I'm not too fussed about 2 spaces (as opposed to tab?) indentation and, as you would expect, 'equals style' (Yihui style was another proposed name) also uses =` assignment. If you point me towards the version of the style you're using I'll take a look.

@pat-s
Copy link
Contributor

pat-s commented Oct 19, 2019

Here is the repo for the mlr-style.

@loganknecht
Copy link

loganknecht commented Mar 2, 2020

Hello!

I am a programmer working in an existing R codebase that is not consistent and I'm looking for some easy wins to get it to a consistent styling.

This appears to be a wonderful package, but it is very unclear how to support custom stylings that can be shared across the team environment.

I have just integrated a .lintr file in our project directory and that was wonderful
https://github.com/jimhester/lintr

However I see no equivalent for how to do so in styler

Do you have guidance on how to support custom configurations AND configure it such that other teams working in this repository will default to those settings?

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Mar 2, 2020

First, in case you have not seen these, I point to a few existing resources related to your question of creating a new style guide:

If you read this thread carefully, it becomes evident that there is no final solution to this problem. Maybe not yet. The cleanest would probably be to create a styler.infra package so people can create standalone R packages like one-liner, but that would be a lot of work on our side.

In any case, we don't really recommend creating your own style guide unless you have very good reasons for it. Since the tidyverse style guide is very popular and in our perspective elegant, we recommend to use it in almost all projects. Also note that it is non-invasive and you have a few options to customise it in many ways. Check ?tidyverse_style for details. Since v.1.3.0, we also support ignoring some lines for styling, since v1.2.0, alignment detection is supported for function calls.

Sharing of custom style guides is also a bit difficult.

  • You can customise the Addin as discussed in ?styler_addins, but this won't affect functions like style_pkg(). So for those, you'd need to supply your style guide every time you call the function.
  • If you create a fork, like the mlr style guide referenced above, after installation of the fork, people can use the style guide like for mlr: styler::style_text("a = 1", style = styler::mlr_style).
  • You can enforce styling upon committing with https://github.com/lorenzwalthert/precommit, which I believe is the preferred way of using styler - and there you can specify which style from which package to use.
  • We have a discussion to store the arguments to style_file() and friends in a config, see storing styler config in a config file / DESCRIPTION / other  #319.

@lorenzwalthert
Copy link
Collaborator

I think it's documented now in #846.

@Robinlovelace
Copy link

Robinlovelace commented Aug 7, 2022

Heads-up anyone following this and @yihui, and @pat-s in particular I've created a minimal {styler.equals} package building on a nice template by @lorenzwalthert: https://github.com/Robinlovelace/styler.equals

There's also this: https://github.com/ropensci-review-tools/spaceout

Not sure if these can go on CRAN due to use of styler::: calls, any ideas on that welcome and, more broadly, any ideas for what the style guide should contain, beyond =? Original plan was for it to match the Geocomputation with R style guide but that is essentially tidyverse style. Many thanks!

@MichaelChirico
Copy link
Contributor

Not sure if these can go on CRAN due to use of styler::: calls

most natural seems to be requesting export of key helper functions :)

@Robinlovelace
Copy link

Was wondering if that would be a possibility and have opened an issue: #974. Thanks for the suggestion Michael!

Robinlovelace added a commit to Robinlovelace/styler.equals that referenced this issue Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants