mrkaye97 / slackr

An R package for sending messages from R to Slack
https://matthewrkaye.com/slackr/
Other
307 stars 83 forks source link

Overhaul how `slackr` and `slackr_bot` post text to Slack #156

Closed mrkaye97 closed 3 years ago

mrkaye97 commented 3 years ago

Per some discussion with @jennybc, I think I can rely on reprex::reprex_slack() and maybe knitr/rmarkdown to overhaul how slackr parses and evaluates the expressions it takes as input to create the Slack API request.

That'll clean up a lot of the sink() logic that's hard to follow and probably unnecessary

mrkaye97 commented 3 years ago

Alright @jennybc, I've thought about using reprex_slack a bit more, and here's the issue I can't seem to get past: I really need the user to be able to pass basically anything into slackr_*(), and often times those things being passed in to slackr are living in the global environment and would need to be recreated inside of a reprex for that approach to work. Or at least as far as I can tell.

For example, you might want to use slackr to send some CSVs to Slack like this:

library(dplyr)
iris_sample <- iris %>% sample_n(10)
mtcars_sample <- mtcars %>% sample_n(10)

## ... do some other stuff ... 

slackr_csv(iris_sample)
slackr_csv(mtcars_sample)

In this case, I can't think of a way to rely on reprex_slack to do the lifting inside of slackr for me without needing a user to pass all of the logic to create iris_sample and mtcars_sample, which would also presumably include loading dplyr. AFAIK, there's no way with reprex to do something like:

iris_sample <- iris %>% sample_n(10)
mtcars_sample <- mtcars %>% sample_n(10)

## ... do some other stuff ...

reprex()

## or

reprex({iris_sample, mtcars_sample})

since that seems like it'd defeat the purpose of reprex. Let me know if I'm wrong in my understanding of that.

I figure I could also copy some of the reprex source -- especially the bits writing to an R file and then converting that R file into markdown ` and have it work perfectly for my needs, but I'm not sure if that'd necessarily be a whole lot easier than just doing the currentsink()stuff. What do you think? Is there an easy way to recycle thereprex` logic using the temporary R file and then markdown file that I'm missing?

jennybc commented 3 years ago

Have you looked into the unexported prex_*() functions?

https://reprex.tidyverse.org/news/index.html#implementation-and-internals

prex(), prex_VENUE(), and prex_render() are new unexported functions that, like reprex(), render a small bit of code, but with much less reproducibility! The code is evaluated in the global workspace of the current process, with the current working directory. This pragmatic hack is useful when preparing a series of related snippets, e.g., for a Keynote or PowerPoint presentation, and there’s not enough space to make each one self-contained.

Sounds like your needs might overlap with this mentality.

And again, don't take me too seriously with my original suggestion, if it feels like it's more trouble than it's worth to use knitr (one way or another).

mrkaye97 commented 3 years ago

Sorry for taking so long to circle back here -- busy week. Thanks for this suggestion. I hadn't read the changelog so didn't realize these prex functions were an option :grimacing:

Will look into it some more and report back

mrkaye97 commented 3 years ago

So I've looked into the prex stuff some more. Here are the issues I'm having:

  1. It seems that reprex:::prex doesn't like running inside of a function for some reason. Here's a (sorta reproducible) example:
> foo <- function(bar) {
+     reprex:::prex(bar)
+ }
> 
> result <- foo(iris)
✓ Preparing reprex as `.R` file:
  'nerdy-ram_reprex.R'
ℹ Rendering reprex...
✓ Writing reprex file:
  'file/path/reprex/nerdy-ram_reprex.md'
✓ Reprex output is on the clipboard.
> 
> result
[1] "``` r"                                                         "bar"                                                           "#> Error in eval(expr, envir, enclos): object 'bar' not found"
[4] "```"                                                          
> 

Not really sure what gives. It must just be looking in the global env for bar and not in the frame where prex is actually being called?

  1. I can't figure out a way to set up something like this:
foo <- function(...) {
  do some stuff to get things to evaluate from the dots

  for item in dots {
     prex(item)
  }
}

foo(head(iris))

and have it give me output like this:

head(iris)
#>   Sepal.Length Sepal.Width Petal.Length Petal.Width Species
#> 1          5.1         3.5          1.4         0.2  setosa
#> 2          4.9         3.0          1.4         0.2  setosa
#> 3          4.7         3.2          1.3         0.2  setosa
#> 4          4.6         3.1          1.5         0.2  setosa
#> 5          5.0         3.6          1.4         0.2  setosa
#> 6          5.4         3.9          1.7         0.4  setosa

instead of like this:

item
#>   Sepal.Length Sepal.Width Petal.Length Petal.Width Species
#> 1          5.1         3.5          1.4         0.2  setosa
#> 2          4.9         3.0          1.4         0.2  setosa
#> 3          4.7         3.2          1.3         0.2  setosa
#> 4          4.6         3.1          1.5         0.2  setosa
#> 5          5.0         3.6          1.4         0.2  setosa
#> 6          5.4         3.9          1.7         0.4  setosa

I figure there's a way around this, but it's not immediately clear to me how to achieve it. In slackr, I cat(thing_to_eval) before actually evaluating it, so I get something that looks like the first one.

Let me know if you've got any other ideas @jennybc!

If I can figure out these two things, it seems like I should be able to plug in prex_slack to replace a bunch of the nasty sink stuff, which would be great!

jennybc commented 3 years ago

Re: the first point, by default, both reprex() and prex() interpret the primary input as an unquoted expression. They're optimized for convenience of an interactive call.

So, yes, they're not really designed for easy wrapping inside another function.

Maybe there's a better way to do this, but here's one way for your wrapper to take such an expression and pass it through:

goo <- function(bar) {
  bar_expr <- substitute(bar)
  eval(rlang::call2(reprex:::prex, bar_expr))
}

Then

goo(head(iris))

yields

head(iris)
#>   Sepal.Length Sepal.Width Petal.Length Petal.Width Species
#> 1          5.1         3.5          1.4         0.2  setosa
#> 2          4.9         3.0          1.4         0.2  setosa
#> 3          4.7         3.2          1.3         0.2  setosa
#> 4          4.6         3.1          1.5         0.2  setosa
#> 5          5.0         3.6          1.4         0.2  setosa
#> 6          5.4         3.9          1.7         0.4  setosa

and

x <- 1:5
goo(x + 1)

yields

x + 1
#> [1] 2 3 4 5 6

Another way to go is to do what reprex itself does, which is to convert such an expression to it's string representation.

hoo <- function(bar) {
  bar_expr <- substitute(bar)
  bar_string <- reprex:::stringify_expression(bar_expr)
  if (length(bar_string == 1)) {
    bar_string <- paste0(bar_string, "\n")
  }
  reprex:::prex(input = bar_string)
}

Again, I'm not sure it you should use reprex. But it sure does seem like it would be nice for you to find a way to use knitr, vs sink().

mrkaye97 commented 3 years ago

Gotcha, okay. I think this approach will work. I think I can just do something like this:

    # get the stuff to send to slack
    args <- substitute(list(...))[-1L]

    x <- purrr::map(
      args,
      ~ eval(rlang::call2(reprex:::prex, .x, html_preview = FALSE, render = TRUE))
    )

maybe with a little bit of help from purrr::quietly to silence the messages printed by prex. One other issue I'm having is that it seems like there's nothing I can do to prevent prex from saving stuff to the working directory. I can probably hack my way around this by finding the name of the files being generated (e.g. white-boto_reprex) and then just removing anything matching that pattern, but that doesn't feel like the sexiest solution to me. Is there a cleaner way around that issue? I tried input = tempdir() and input = tempfile() to no avail.

Regardless, AFAICT using reprex to call knitr makes a lot more sense than trying to get knitr to do what I want without reprex, since I think I'd need to set up my own scaffolding in that case (which you've already done).

jennybc commented 3 years ago

I did recently change this in reprex, so make sure you're developing against dev reprex:

https://github.com/tidyverse/reprex/commit/2e043e0c2fbfa779b6aee356749acf36cfb90da2

https://github.com/tidyverse/reprex/commit/5cf02b4b5b826904db30ce590f08a5441535098b

mrkaye97 commented 3 years ago

Circling back. I think I have it working! Many thanks again for all the help @jennybc. I've linked the changes in #159 to this issue.

Here is the meat of this change: https://github.com/mrkaye97/slackr/pull/159/files#diff-541ec58513b76ed3c75296b18862b5ade6fbac59cec714bef1d80e6abecf5de6L55-R96

I think it's much cleaner this way.

Last question for you: I figure I'll start failing CMD check on CRAN if I reprex:::prex(). Is there a best practice for using unexported functions from reprex in slackr without either you exporting them or me making a copy of them? I guess I could utils::getFromNamespace("prex", "reprex") (which is what I'm doing now for testing).

jennybc commented 3 years ago

Nice! Yeah I think the way you are accessing prex() is one of the best options. I see the you "de-markdownize" it, so I wonder if you'd be happier with prex_r()?

mrkaye97 commented 3 years ago

Yep, sounds like a great idea! Thanks again :)