Gaps in documentation for custom style guides

sorhawell commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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.

sorhawell commented 9 months ago

@lorenzwalthert In yesterday's confirmation I accidentally changed to create_style_guide()

It appears that even when changing the name and version there is some unintended caching which cannot be disabled.

After running tidyverse_style() a custom style cannot be used again.

# 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
  }
  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()) 
#> 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())
#> Styling  1  files:
#>  deleteme.R ✔ 
#> ────────────────────────────────────────
#> Status   Count   Legend 
#> ✔    1   File unchanged.
#> ℹ    0   File changed.
#> ✖    0   Styling threw an error.
#> ────────────────────────────────────────
cat(readLines("deleteme.R"),sep = "\n") # bad
#> foo <- 42
#> bar <- TRUE
#> f <- function() {
#>   1
#> }
#> if (f()) {
#>   print("one")
#> }

styler::cache_deactivate()
#> Deactivated cache.
styler::cache_clear(ask = FALSE)
styler::style_file("deleteme.R", transformers = rpolars_style())
#> Styling  1  files:
#>  deleteme.R ✔ 
#> ────────────────────────────────────────
#> Status   Count   Legend 
#> ✔    1   File unchanged.
#> ℹ    0   File changed.
#> ✖    0   Styling threw an error.
#> ────────────────────────────────────────
cat(readLines("deleteme.R"),sep = "\n") # bad
#> foo <- 42
#> bar <- TRUE
#> f <- function() {
#>   1
#> }
#> if (f()) {
#>   print("one")
#> }

^{Created on 2023-12-11 with reprex v2.0.2}

sorhawell commented 9 months ago

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

sorhawell commented 9 months ago

I should have read this part about updating name and version: https://styler.r-lib.org/articles/customizing_styler.html#cache-invalidation

sorhawell commented 9 months ago

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 commented 9 months ago

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

r-lib / styler

Gaps in documentation for custom style guides #1170