"No visible binding for global variable"

[BenjaminWolfe]

Thank you for creating factory! I used it as suggested when creating my signs package this weekend: https://benjaminwolfe.github.io/signs/

When I run devtools::check() on my package, I get the following warning:

no visible binding for global variable 'x'
   Undefined global functions or variables:
     x

My package creates a function factory, signs_format(), that outputs a function that will take a single argument, x. It's a pretty simple package, so if it isn't too cheeky I submit the whole thing as my reprex. The code I wrote with factory is here.

  rlang::new_function(
    as.pairlist(alist(x = )),
    rlang::expr({
      signs(x                  = x,
            !!!dots,
            format             = !!format,
            add_plusses        = !!add_plusses,
            trim_leading_zeros = !!trim_leading_zeros,
            label_at_zero      = !!label_at_zero)
      }),
    rlang::caller_env()
  )

I'm not sure whether it's balking at that precisely, or at where I use it in my roxygen2 examples...

#' x <- seq(-5, 5)
#' f1 <- signs_format()
#' f1(x)

...or at my explicit tests:

library(signs)
x <- seq(-1, 1)

test_that(
  "the basics work",
  {
    expect_equal(
      signs_format()(x),
      c("\u22121", "0", "1")
    )
  }
)

[jonthegeek]

It's definitely the spot where x is bare in the factory that's causing the error (or, well, somewhere else where x is used without first defining it). I think I know roughly how to fix it, I'll just have to sit down and play a little. Thanks for using the package and finding this issue!

[BenjaminWolfe]

For now, I notice that adding the file R/globals.R with a single line of utils::globalVariables("x"), per this community post, removes the devtools::check() note.

I'm not sure that's the best solution though.

[jonthegeek]

Yeah, that "fixes" it, but I'm not sure what CRAN thinks about hacks like that. We should be able to fix it for real, I just need to try a couple things.

[BenjaminWolfe]

There's also a hilarious little thread on StackOverflow with Hadley re: that particular workaround. :)

https://stackoverflow.com/a/12429344/573332

[jonthegeek]

Yup, that's why most of my work packages currently have a "01_fix_global_warning.R" file, but it SHOULDN'T be necessary here 😁

[BenjaminWolfe]

I love that I first read that as “fix global warming.” I'm like, that's an ambitious package.

[jonthegeek]

@BenjaminWolfe Do you have the call to factory handy (or can you reproduce it)?

[BenjaminWolfe]

Let me know if this helps!

First, I wrote the signs function (see code). Then I ran the following factory code to get the ball rolling (I believe this is the call):

signs_format <- build_factory(
  fun = function(x) {
    signs(
      x,
      format             = format,
      add_plusses        = add_plusses,
      trim_leading_zeros = trim_leading_zeros,
      label_at_zero      = label_at_zero
    )
  },
  format,
  add_plusses,
  trim_leading_zeros,
  label_at_zero
)

Then just running signs_format gave me the following (cleaned up a bit):

function (format, add_plusses, trim_leading_zeros, label_at_zero) {
  rlang::new_function(
    as.pairlist(alist(x = )),
    rlang::expr({
      signs(x,
            format             = `!!`(format),
            add_plusses        = `!!`(add_plusses), 
            trim_leading_zeros = `!!`(trim_leading_zeros), 
            label_at_zero      = `!!`(label_at_zero))
      }),
    rlang::caller_env()
  )
}

To this I added just a few changes.

1. I added dots. So the function definition starts with function(..., format), its first line is dots <- rlang::list2(...), and the rlang::expr() call includes !!!dots.
2. I added default values to my arguments in the definition and a check_args() call defined here. No biggie.
3. Inexplicably, I made the rlang::expr() call say signs(x = x) instead of just signs(x). I'm not sure if I did something different with my original call that stuck that in there, or if I somehow made that change later. Little befuddled on that point.

With those changes, it should match exactly what I ended up putting in the package here. Everything works fine in the package, except I had to add this line of code to avoid an R CMD CHECK warning.

Here's a sample function call. If you have signs installed and attached and you use factory to overwrite signs_format(), this will work just fine. In the console it'll look just like "-1" or "-1.0," unless for some reason you code in a variable-width font. :)

signs_format(
  format             = scales::number,
  add_plusses        = FALSE,
  trim_leading_zeros = FALSE,
  label_at_zero      = "none"
)(-1)

[jonthegeek]

FYI, I haven't sorted out the actual solution to this yet, BUT we do now handle dots better in {factory}! Use the new parameter .pass_dots = TRUE in your build_factory call to pass dots into the function, and put dots in any part(s) of the function that should have them.

I don't think I'll be able to deal with the global variable issue, unfortunately. At least not directly. I'll keep this open to add a warning or something at some point, and I'll try some things out and MAYBE find a fix, but at the moment it "feels" like it won't be fixable.