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

Gaps in documentation for custom style guides #1170

Open
sorhawell opened this issue Dec 11, 2023 · 9 comments
Open

Gaps in documentation for custom style guides #1170

sorhawell opened this issue Dec 11, 2023 · 9 comments

Comments

@sorhawell
Copy link

sorhawell commented Dec 11, 2023

Hi stylers

Thank you for a very useful package. I think I have found 1 or perhaps 2 bugs:

  • It appears that style_text() ignores custom style unless cache is disabled.
  • It appears that style_file() w/o cache has unintended side effects such that it can only work once with a custom style. To work again the session must be restarted
# Prepare transformers
rpolars_style = function() {

  # derive from tidyverse
  transformers = styler::tidyverse_style()

  # reverse tranformer to make <- into =
  transformers$token$force_assignment_op = function (pd) {
    to_replace = pd$token == "LEFT_ASSIGN"
    pd$token[to_replace] = "EQ_ASSIGN"
    pd$text[to_replace] = "="
    pd
  }

  transformers
}


# CACHE does not take into account transformers
styler::style_text("foo <- 42; bar = TRUE", transformers = rpolars_style()) # good
#> foo = 42
#> bar = TRUE
styler::style_text("foo <- 42; bar = TRUE", transformers = styler::tidyverse_style()) # bad
#> foo <- 42
#> bar = TRUE
styler::style_text("foo <- 42; bar = TRUE", transformers = rpolars_style()) # bad
#> foo <- 42
#> bar = TRUE

styler::cache_clear(ask = FALSE)
styler::cache_deactivate()
#> Deactivated cache.
styler::style_text("foo <- 42; bar = TRUE", transformers = rpolars_style()) # good
#> foo = 42
#> bar = TRUE
styler::style_text("foo <- 42; bar = TRUE", transformers = styler::tidyverse_style()) # good
#> foo <- 42
#> bar <- TRUE
styler::style_text("foo <- 42; bar = TRUE", transformers = rpolars_style()) # good
#> foo = 42
#> bar = TRUE



writeLines("foo <- 42; bar = TRUE", "deleteme.R")
readLines("deleteme.R")
#> [1] "foo <- 42; bar = TRUE"
styler::style_file("deleteme.R", transformers = rpolars_style()) # good
#> Styling  1  files:
#>  deleteme.R ℹ 
#> ────────────────────────────────────────
#> Status   Count   Legend 
#> ✔    0   File unchanged.
#> ℹ    1   File changed.
#> ✖    0   Styling threw an error.
#> ────────────────────────────────────────
#> Please review the changes carefully!
readLines("deleteme.R")
#> [1] "foo = 42"   "bar = TRUE"
styler::style_file("deleteme.R", transformers = styler::tidyverse_style()) # good
#> Styling  1  files:
#>  deleteme.R ℹ 
#> ────────────────────────────────────────
#> Status   Count   Legend 
#> ✔    0   File unchanged.
#> ℹ    1   File changed.
#> ✖    0   Styling threw an error.
#> ────────────────────────────────────────
#> Please review the changes carefully!
readLines("deleteme.R")
#> [1] "foo <- 42"   "bar <- TRUE"
styler::style_file("deleteme.R", transformers = rpolars_style()) # bad
#> Styling  1  files:
#>  deleteme.R ✔ 
#> ────────────────────────────────────────
#> Status   Count   Legend 
#> ✔    1   File unchanged.
#> ℹ    0   File changed.
#> ✖    0   Styling threw an error.
#> ────────────────────────────────────────
readLines("deleteme.R")
#> [1] "foo <- 42"   "bar <- TRUE"
styler::cache_clear(ask = FALSE)
styler::cache_deactivate()
#> Deactivated cache.
styler::style_file("deleteme.R", transformers = rpolars_style()) # bad
#> Styling  1  files:
#>  deleteme.R ✔ 
#> ────────────────────────────────────────
#> Status   Count   Legend 
#> ✔    1   File unchanged.
#> ℹ    0   File changed.
#> ✖    0   Styling threw an error.
#> ────────────────────────────────────────
readLines("deleteme.R")
#> [1] "foo <- 42"   "bar <- TRUE"

Created on 2023-12-11 with reprex v2.0.2

@sorhawell
Copy link
Author

sorhawell commented Dec 11, 2023

styler 1.10.2


platform       x86_64-apple-darwin20       
arch           x86_64                      
os             darwin20                    
system         x86_64, darwin20            
status                                     
major          4                           
minor          3.0                         
year           2023                        
month          04                          
day            21                          
svn rev        84292                       
language       R                           
version.string R version 4.3.0 (2023-04-21)
nickname       Already Tomorrow  

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Dec 11, 2023

Thanks. I believe that may happen because the style guide Name and version that {styler} uses to distinguish between style guides has not been overwritten. Hence, it’s impossible to distinguish your style guide from the tidyverse style when checking the cache. Please see the docs for that: https://styler.r-lib.org/reference/create_style_guide.html.

If you don’t create a style guide from scratch, you can just overwrite these attributes in the list returned by tidyverse_style() I believe. Can you try that? In any case, we should probably fix the docs in multiple places to account for that.

@sorhawell
Copy link
Author

Thank you @lorenzwalthert . That indeed work perfectly.

I think what went wrong for me is that this guide is focused on making a new style from scratch
vignette("customizing_styler").

... and this guide focuses on deriving, but did not mention the style_guide_name requirement as far as I can see
vignette("remove rules")

Thank you very much !
From my point of view this issue can be closed.

# Prepare transformers
rpolars_style = function() {

  # derive from tidyverse
  transformers = styler::create_style_guide()
  transformers$style_guide_name = "rpolars_style"
  transformers$style_guide_version = "0.1.0"

  # reverse tranformer to make <- into =
  transformers$token$force_assignment_op = function (pd) {
    to_replace = pd$token == "LEFT_ASSIGN"
    pd$token[to_replace] = "EQ_ASSIGN"
    pd$text[to_replace] = "="
    pd
  }

  transformers
}


# CACHE does not take into account transformers
styler::style_text("foo <- 42; bar = TRUE", transformers = rpolars_style()) # good
#> foo = 42; bar = TRUE
styler::style_text("foo <- 42; bar = TRUE", transformers = styler::tidyverse_style()) # good
#> foo <- 42
#> bar <- TRUE
styler::style_text("foo <- 42; bar = TRUE", transformers = rpolars_style()) # good
#> foo = 42; bar = TRUE

writeLines("foo <- 42; bar = TRUE", "deleteme.R")
readLines("deleteme.R")
#> [1] "foo <- 42; bar = TRUE"
styler::style_file("deleteme.R", transformers = rpolars_style()) # good
#> Styling  1  files:
#>  deleteme.R ℹ 
#> ────────────────────────────────────────
#> Status   Count   Legend 
#> ✔    0   File unchanged.
#> ℹ    1   File changed.
#> ✖    0   Styling threw an error.
#> ────────────────────────────────────────
#> Please review the changes carefully!
readLines("deleteme.R")
#> [1] "foo = 42; bar = TRUE"
styler::style_file("deleteme.R", transformers = styler::tidyverse_style()) # good
#> Styling  1  files:
#>  deleteme.R ℹ 
#> ────────────────────────────────────────
#> Status   Count   Legend 
#> ✔    0   File unchanged.
#> ℹ    1   File changed.
#> ✖    0   Styling threw an error.
#> ────────────────────────────────────────
#> Please review the changes carefully!
readLines("deleteme.R")
#> [1] "foo <- 42"   "bar <- TRUE"
styler::style_file("deleteme.R", transformers = rpolars_style()) # good
#> Styling  1  files:
#>  deleteme.R ℹ 
#> ────────────────────────────────────────
#> Status   Count   Legend 
#> ✔    0   File unchanged.
#> ℹ    1   File changed.
#> ✖    0   Styling threw an error.
#> ────────────────────────────────────────
#> Please review the changes carefully!
readLines("deleteme.R")
#> [1] "foo = 42"   "bar = TRUE"

Created on 2023-12-11 with reprex v2.0.2

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Dec 11, 2023

Glad it works. I think the docs need refactoring.

  • There should be a vignette called Customizing styling that outlines the three avenues of customization, as they are outlined in the Remove rules vignette with cross-links to the following other vignettes.
  • The Remove rules vignette should be called Derive from existing style guide and terminology should not exclusively be on removal. Should mention caching topic with style guide name and version as well as transformers to drop.
  • The Customizing styler vignette should be called Create style guide from scratch. Should mention caching topic with style guide name and version. Plus transformers to drop.

The should both mention the vignette on distributing style guides. We should be aware that renaming the vignettes will invalidate all links to those and maybe we can use pkgdown redirect feature.

@lorenzwalthert lorenzwalthert changed the title styler cache prevents custom styling Gaps in documentation Dec 11, 2023
@lorenzwalthert lorenzwalthert changed the title Gaps in documentation Gaps in documentation for custom style guides Dec 11, 2023
@sorhawell

This comment was marked as outdated.

@sorhawell
Copy link
Author

I will try to figure something out by reading guidelines and a bit of source carefully :)

@sorhawell

This comment was marked as duplicate.

@sorhawell
Copy link
Author

sorhawell commented Dec 11, 2023

Now it works ... again :)

The Remove rules vignette should be called Derive from existing style guide and terminology should not exclusively be on removal.

yes and also transformer_drops was needed in my case

example

# Prepare transformers
rpolars_style = function() {
  
  # derive from tidyverse
  transformers = styler::tidyverse_style()
  transformers$style_guide_name = "rpolars_style"
  transformers$style_guide_version = "0.1.0"

  # reverse tranformer to make <- into =
  transformers$token$force_assignment_op = function (pd) {
    to_replace = pd$token == "LEFT_ASSIGN"
    pd$token[to_replace] = "EQ_ASSIGN"
    pd$text[to_replace] = "="
    pd
  }
  
  # Specify which tokens must be absent for a transformer to be dropped
  # https://styler.r-lib.org/reference/specify_transformers_drop.html?q=transformers%20_%20drop
  transformers$transformers_drop$token$force_assignment_op = "LEFT_ASSIGN"
  
  
  transformers
}


writeLines("
foo <- 42; bar = TRUE
f = function() {
  1
}
if(f()) {
  print('one')
}
", "deleteme.R")
cat(readLines("deleteme.R"),sep = "\n")
#> 
#> foo <- 42; bar = TRUE
#> f = function() {
#>   1
#> }
#> if(f()) {
#>   print('one')
#> }

styler::style_file("deleteme.R", transformers = rpolars_style(), style = rpolars_style) 
#> Styling  1  files:
#>  deleteme.R ℹ 
#> ────────────────────────────────────────
#> Status   Count   Legend 
#> ✔    0   File unchanged.
#> ℹ    1   File changed.
#> ✖    0   Styling threw an error.
#> ────────────────────────────────────────
#> Please review the changes carefully!
cat(readLines("deleteme.R"),sep = "\n") # good
#> foo = 42
#> bar = TRUE
#> f = function() {
#>   1
#> }
#> if (f()) {
#>   print("one")
#> }

styler::style_file("deleteme.R", transformers = styler::tidyverse_style())
#> Styling  1  files:
#>  deleteme.R ℹ 
#> ────────────────────────────────────────
#> Status   Count   Legend 
#> ✔    0   File unchanged.
#> ℹ    1   File changed.
#> ✖    0   Styling threw an error.
#> ────────────────────────────────────────
#> Please review the changes carefully!
cat(readLines("deleteme.R"),sep = "\n") # good
#> foo <- 42
#> bar <- TRUE
#> f <- function() {
#>   1
#> }
#> if (f()) {
#>   print("one")
#> }

styler::style_file("deleteme.R", transformers = rpolars_style(), style = rpolars_style)
#> Styling  1  files:
#>  deleteme.R ℹ 
#> ────────────────────────────────────────
#> Status   Count   Legend 
#> ✔    0   File unchanged.
#> ℹ    1   File changed.
#> ✖    0   Styling threw an error.
#> ────────────────────────────────────────
#> Please review the changes carefully!
cat(readLines("deleteme.R"),sep = "\n") # good
#> foo = 42
#> bar = TRUE
#> f = function() {
#>   1
#> }
#> if (f()) {
#>   print("one")
#> }

Created on 2023-12-11 with reprex v2.0.2

@lorenzwalthert
Copy link
Collaborator

Ok great. Yeah true, updating which transformers should be dropped is also required, I completely forgot about that.

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

No branches or pull requests

2 participants