Call capture.output() instead of getSrcref() to read design code

[clarabicalho]

This addresses #50 by generating the same output for designer code attribute as before, except it does not depend on keep_source during package install to work.

Shinyapps.io also doesn't seem to support options when building dependencies, so moving away from getSrcref() allows shiny.io server to install and run designer function calls without issues (currently app outputs relying on design call are returning errors).

[nfultz]

Capture.output relies on print, see deparse instead

…

On Wed, May 23, 2018, 7:40 AM Clara Bicalho ***@***.***> wrote: This addresses #50 <#50> by generating the same output for designer code attribute as before, except it does not depend on keep_source during package install to work. Shinyapps.io also doesn't seem to support options when building dependencies, so moving away from getSrcref() allows shiny.io server to install and run designer function calls without issues (currently app outputs relying on design call are returning errors). ------------------------------ You can view, comment on, or merge this pull request online at: #61 Commit Summary - Call capture.output() instead of getSrcref() to read design code File Changes - *M* R/helpers.R <https://github.com/DeclareDesign/DesignLibrary/pull/61/files#diff-0> (2) Patch Links: - https://github.com/DeclareDesign/DesignLibrary/pull/61.patch - https://github.com/DeclareDesign/DesignLibrary/pull/61.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#61>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZjTshjToxXIEV4VP1ZPDyW9oAHbs8tks5t1XT0gaJpZM4UKlur> .

[coveralls]

Pull Request Test Coverage Report for Build 242

• 8 of 18 (44.44%) changed or added relevant lines in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-2.4%) to 94.03%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
R/helpers.R	8	18	44.44%

Files with Coverage Reduction	New Missed Lines	%
R/helpers.R	1	61.76%

Totals
Change from base Build 240:	-2.4%
Covered Lines:	378
Relevant Lines:	402

---

💛 - Coveralls

[jaspercooper]

This is exciting, thanks.

I could see an argument for staying with capture.output() over deparse():

test <- function(){
  "don't grab this code"
  {{{
    "grab this code"
  }}}
  return()
}

txt_deparse <- deparse(test)
txt_capture <- capture.output(test)

# This breaks due to line-splitting
grep("[{]{3}", txt_deparse)
# This does not break
grep("[{]{3}", txt_capture)

However, both break here so we need to solve the line-splitting problem:

test <- function() {
  "don't grab this code"
    {
      {
        {
          "grab this code"
        }
      }
    }
    return()
  }

txt_deparse <- deparse(test)
txt_capture <- capture.output(test)

grep("[{]{3}", txt_deparse)
grep("[{]{3}", txt_capture)

[nfultz]

capture.output calls the print.function method implicitly, which uses the getsrcref if its' there and deparse if it isn't - see https://github.com/wch/r-source/blob/fe7d9b54b2f56fcae876603617aebc668fef68cc/src/main/print.c#L184 You should choose one and stick to it, otherwise the output could depend on the environment (eg some users see the comments in the design and some don't)

…

On Wed, May 23, 2018 at 8:13 AM, Jasper Cooper ***@***.***> wrote: This is exciting, thanks. I could see an argument for staying with capture.output() over deparse(): test <- function(){ "don't grab this code" {{{ "grab this code" }}} return() } txt_deparse <- deparse(test)txt_capture <- capture.output(test) # This breaks due to line-splitting grep("[{]{3}", txt_deparse)# This does not break grep("[{]{3}", txt_capture) However, both break here so we need to solve the line-splitting problem: test <- function() { "don't grab this code" { { { "grab this code" } } } return() } txt_deparse <- deparse(test) txt_capture <- capture.output(test) grep("[{]{3}", txt_deparse) grep("[{]{3}", txt_capture) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#61 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZjTjT9C5BtTKDwJ7aBpYLup-WZu3IRks5t1XymgaJpZM4UKlur> .

[nfultz]

Another alternative is to revert from the getSrcRef version to the original version of the helper function, which unpacked the function body programatically - comments won't be available though, which was why it was changed.

…

On Wed, May 23, 2018 at 8:18 AM, Neal Fultz ***@***.***> wrote: capture.output calls the print.function method implicitly, which uses the getsrcref if its' there and deparse if it isn't - see https://github.com/wch/r-source/blob/fe7d9b54b2f56fcae876603617aebc 668fef68cc/src/main/print.c#L184 You should choose one and stick to it, otherwise the output could depend on the environment (eg some users see the comments in the design and some don't) On Wed, May 23, 2018 at 8:13 AM, Jasper Cooper ***@***.***> wrote: > This is exciting, thanks. > > I could see an argument for staying with capture.output() over deparse(): > > test <- function(){ > "don't grab this code" > {{{ > "grab this code" > }}} > return() > } > txt_deparse <- deparse(test)txt_capture <- capture.output(test) > # This breaks due to line-splitting > grep("[{]{3}", txt_deparse)# This does not break > grep("[{]{3}", txt_capture) > > However, both break here so we need to solve the line-splitting problem: > > test <- function() { > "don't grab this code" > { > { > { > "grab this code" > } > } > } > return() > } > > txt_deparse <- deparse(test) > txt_capture <- capture.output(test) > > grep("[{]{3}", txt_deparse) > grep("[{]{3}", txt_capture) > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#61 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAZjTjT9C5BtTKDwJ7aBpYLup-WZu3IRks5t1XymgaJpZM4UKlur> > . >

[jaspercooper]

Awesome, thanks so much for that clarification Neal. I see your point about the environment.

Any drawbacks to using deparse(), aside from the splitting (which is easy enough to work around)? It's not using the source code at all?

[nfultz]

internally, each function body is stored as an expression tree object - `deparse` converts that back to text - that means comments and whitespace can change. The reason why each `{` gets its own line with deparse is that each one is a separate node in the expression tree, and R prints a new line for each one.

…

On Wed, May 23, 2018 at 8:27 AM, Jasper Cooper ***@***.***> wrote: Awesome, thanks so much for that clarification Neal. I see your point about the environment. Any drawbacks to using deparse(), aside from the splitting (which is easy enough to work around)? It's not using the source code at all? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#61 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZjTqV2shdHWRkG28wQ3QmTHsYJPm60ks5t1X_hgaJpZM4UKlur> .

[jaspercooper]

OK that makes sense and also explains the comment issue. I hadn't noticed deparse() was ignoring comments.

So either:

1. we rely on source code and have comments, at the cost of lots of install problems
2. we use an expression tree approach, but lose comments because they're not part of the tree
3. we rely on capture.output which will not have comments when it doesn't have the source, but should otherwise work OK, with the possible issue of changes to the environment

If I've accurately summarized the choices we face, I'd vote for 3 I think

Edit: I tested building the package using capture.output() and not keeping source, and it doesn't work.

[nfultz]

For behavior of number 3, strongly recommend writing your own helper instead of relying on `capture.output(print(design_template))` - print is generic and can be overrode, for example if people have crayon / colorout loaded there can be control codes for color or other garbage in the output. We should check if the srcref attribute is present, if so use it, otherwise fall back to deparse.

…

On Wed, May 23, 2018 at 9:27 AM, Jasper Cooper ***@***.***> wrote: OK that makes sense and also explains the comment issue. I hadn't noticed deparse() was ignoring comments. So either: 1. we rely on source code and have comments, at the cost of lots of install problems 2. we use an expression tree approach, but lose comments because they're not part of the tree 3. we rely on capture.output which will not have comments when it doesn't have the source, but should otherwise work OK, with the possible issue of changes to the environment If I've accurately summarized the choices we face, I'd vote for 3 I think — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#61 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZjTrbqsfMAVdZIgO5ra_RBTujABCh2ks5t1Y32gaJpZM4UKlur> .

[clarabicalho]

I use getSrcref() and deparse() if source file is not available. Built from this branch and ran fine. I don't see the advantage of having it both ways over using deparse() in all cases though.

[jaspercooper]

OK I'm currently running some tests of the keep.source and not keep.source versions of the build.

I'm a little bit sad we have lost comments using deparse.

[nfultz]

If you are falling back to deparse, you should locate the {{{ block in the expression tree directly instead of using a regex, like in the original version of this function.

f <- function(x){
  {FALSE}
  {{{
    step1 <- a
    step2 <- b
    step3 <- c
    step4 <- z
    design <- step1 / step2 / step3 / step4
    
  }}}
  {{FALSE}}
  {FALSE}
  x
}


find_triple_bracket <- function(f){
  
  pred <- function(expr, depth=3) {
    (depth == 0) || (
      length(expr) > 1 &&
        expr[[1]] == as.symbol('{') &&
        Recall(expr[[2]], depth - 1)
    )
  }
  
  clean <- function(ch, n=length(ch)-1) paste0(ch[2:n], collapse = "\n")
  
  ret <- Filter(pred, body(f))
  
  if(length(ret) == 0) "" else clean(deparse(ret[[1]][[2]][[2]]))
  
}

find_triple_bracket(f)

[nfultz]

Also on shinyapps.io, did you try setting the R_KEEP_PKG_SOURCE in Renviron or Renviron.site? It might get picked up that way- https://stackoverflow.com/questions/39084284/how-to-pass-environment-variables-to-shinyapps

[clarabicalho]

On shinyapps.io question: yes, I tried that and did not work. Is setting R_KEEP_PKG_SOURCE=yes in .Renviron enough on a Windows computer? It seems like ?Startup suggests a different default directory for .Renviron and .Rprofile, but perhaps I'm misreading it: "On systems with sub-architectures (mainly Windows), the files ‘Renviron.site’ and ‘Rprofile.site’ are looked for first in architecture-specific directories, e.g. ‘R_HOME/etc/i386/Renviron.site’. And e.g. ‘.Renviron.i386’ will be used in preference to ‘.Renviron’." Thanks for pointing to find_triple_bracket(), very helpful and much neater than regex. 2018-05-25 0:00 GMT+02:00 Neal Fultz <notifications@github.com>:

…

Also on shinyapps.io, did you try setting the R_KEEP_PKG_SOURCE in Renviron or Renviron.site? It might get picked up that way- https://stackoverflow.com/questions/39084284/how-to- pass-environment-variables-to-shinyapps — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#61 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYa0JBIwCwADVj9lT8udjTogTprxK4L4ks5t1y2EgaJpZM4UKlur> .

[nfultz]

I would recommend opening a support ticket for shinyapps and see if they have a way of getting the env vars set at install time instead of just run time.

[jaspercooper]

OK I'm going to merge this but have to fix some of the tests that have been broken with updates to diagnose_design

[jaspercooper]

All looks good

[jaspercooper]

Can we name x something more informative and also add some comments explaining what is happening at each step here?

[jaspercooper]

Is this necessarily a problem to have more than 3? totally fine to leave it in there, but a priori I can't see the problem in including anything between the last three braces in {{{{{ and the first three braces in }}}}}.

[jaspercooper]

OK this is ready to go