Skip to content

Chore: standardize one-liner ggproto methods to use { #6459

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

Open
yjunechoe opened this issue May 15, 2025 · 1 comment · May be fixed by #6460
Open

Chore: standardize one-liner ggproto methods to use { #6459

yjunechoe opened this issue May 15, 2025 · 1 comment · May be fixed by #6460

Comments

@yjunechoe
Copy link
Contributor

yjunechoe commented May 15, 2025

There are some inconsistencies in how one-liner ggproto methods are written, with and without {. As far as I can tell, { for one-liners seems like an arbitrary stylistic choice in the source code

library(ggplot2)

Geom$setup_data
#> <ggproto method>
#>   <Wrapper function>
#>     function (...) 
#> setup_data(...)
#> 
#>   <Inner function (f)>
#>     function (data, params) 
#> data

Stat$setup_data
#> <ggproto method>
#>   <Wrapper function>
#>     function (...) 
#> setup_data(...)
#> 
#>   <Inner function (f)>
#>     function (data, params) 
#> {
#>     data
#> }

It's my understanding that starting a function body with { is more standard. It lets methods show up on the RStudio outline pane and also some debugging functions require it (ex: trace() with an at argument).

A quick search finds a few hits for one-liners without { like Geom$setup_data:

library(ggplot2)
ggprotos <- mget(ls("package:ggplot2"), asNamespace("ggplot2"), ifnotfound = list(NULL)) |>
  Filter(\(x) inherits(x, "ggproto"), x = _)
ggproto_methods_nobrackets <- lapply(ggprotos, \(x) {
  Filter(
    \(m) inherits(x[[m]], "ggproto_method") &&
      length(body(get(m, x))) == 1,
    ls(envir = x)
  )
})
sum(lengths(ggproto_methods_nobrackets))
#> [1] 19
Filter(length, ggproto_methods_nobrackets)
#> $Coord
#> [1] "is_free"   "is_linear"
#> 
#> $CoordCartesian
#> [1] "is_free"   "is_linear"
#> 
#> $CoordFixed
#> [1] "is_free"
#> 
#> $CoordPolar
#> [1] "aspect"  "is_free"
#> 
#> $CoordRadial
#> [1] "is_free"
#> 
#> $CoordSf
#> [1] "is_free" "labels" 
#> 
#> $CoordTrans
#> [1] "is_free"
#> 
#> $Geom
#> [1] "setup_data"   "setup_params"
#> 
#> $GeomBlank
#> [1] "draw_panel" "handle_na" 
#> 
#> $ScaleBinned
#> [1] "is_discrete"
#> 
#> $ScaleContinuous
#> [1] "is_discrete"
#> 
#> $ScaleDiscrete
#> [1] "is_discrete" "transform"

This should be a trivial, non-intrusive chore that's easy to unit test. I'm happy to PR this if it seems within scope!

@teunbrand
Copy link
Collaborator

Thanks for the issue June! I believe from our prior discussion that one-liners are also untrace()able, right? In any case, if it is harmless and { is preferred, I see no obstacles for a PR :)

@yjunechoe yjunechoe linked a pull request May 15, 2025 that will close this issue
@teunbrand teunbrand linked a pull request May 15, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants