moodymudskipper / flow

View and Browse Code Using Flow Diagrams
https://moodymudskipper.github.io/flow/
Other
395 stars 26 forks source link

FR: allow flow_view to parse "withr" or "suppress" family of functions #162

Open zkamvar opened 11 months ago

zkamvar commented 11 months ago

I notice that if I have a {withr}-like pattern in my code or use a suppress family function, {flow} cannot disentangle it.

I will occasionally have functions that look like this:

fun <- function(path, ...) {
  # ... 
  withr::with_dir(path, {
    # ... 
  })
  # ...
}

When flow encounters the {withr} call, it does not parse the expression. The same is true for calls to suppressMessages(). As a reproducible example, here is something should produce a simple if/else flow chart:

f <- function() {
  suppressMessages({
    if (getOption("a.test") == 1L) print("1") else print("2")
})
}

flow::flow_view(f)
flowchart with three nodes: the function call, the function body, and the exit state. No if else logic is in there.

Created on 2023-07-10 with reprex v2.0.2

That being said, I really like {flow}! I'm in the process of onboarding new developers and being able to create these visuals to walk through the logic is super helpful!

moodymudskipper commented 11 months ago

I like the idea but I wonder what this would look like and how it would work.

Functions are common offenders for this situation and I implemented the nested_fun argument for those.

We might do something like [this](https://www.nomnoml.com/#view/%23.if%3A%20visual%3Drhomb%20fill%3D%23e2efda%20align%3Dcenter%0A%23.for%3A%20visual%3Drhomb%20fill%3D%23ddebf7%20align%3Dcenter%0A%23.repeat%3A%20visual%3Drhomb%20fill%3D%23fce4d6%20align%3Dcenter%0A%23.while%3A%20visual%3Drhomb%20fill%3D%23fff2cc%20align%3Dcenter%0A%23.standard%3A%20visual%3Dclass%20fill%3D%23ededed%20align%3Dleft%0A%23.standardg%3A%20visual%3Dclass%20fill%3D%2370ad47%20align%3Dleft%0A%23.standardr%3A%20visual%3Dclass%20fill%3D%23ed7d31%20align%3Dleft%0A%23.commented%3A%20visual%3Dclass%20fill%3D%23ededed%0A%23.header%3A%20visual%3Droundrect%20fill%3D%23d9e1f2%20align%3Dleft%0A%23.return%3A%20visual%3Dend%20fill%3D%2370ad47%20%20empty%0A%23.stop%3A%20visual%3Dend%20fill%3D%23ed7d31%20%20empty%0A%23.break%3A%20visual%3Dreceiver%20fill%3D%23ffc000%20empty%0A%23.next%3A%20visual%3Dtransceiver%20fill%3D%235b9bd5%20%20empty%0A%23arrowSize%3A%201%0A%23bendSize%3A%200.3%0A%23direction%3A%20down%0A%23gutter%3A%205%0A%23edgeMargin%3A%200%0A%23edges%3A%20hard%0A%23fill%3A%20%23eee8d5%0A%23fillArrows%3A%20false%0A%23font%3A%20Courier%0A%23fontSize%3A%2012%0A%23leading%3A%201.25%0A%23lineWidth%3A%203%0A%23padding%3A%2016%0A%23spacing%3A%2040%0A%23stroke%3A%20%2333322E%0A%23title%3A%20filename%0A%23zoom%3A%201%0A%23acyclicer%3A%20greedy%0A%23ranker%3A%20network-simplex%0A%0A%5B%3Cheader%3Ef()%5D%20%20-%3E%20%5B%3Cstandard%3E%201%3A%20suppressMessages(%7B👇%7D)%0A%5B%3Cif%3E%201%3A%20%3B⠀%20if%20(getOption(%22a.test%22)%20%3D%3D%201L)%20⠀%0A⠀%20⠀%20⠀%5D%0A%5B%3Cif%3E%201%3A%20%3B⠀%20if%20(getOption(%22a.test%22)%20%3D%3D%201L)%20⠀%0A⠀%20⠀%20⠀%5D%20y%20-%3E%20%5B%3Cstandard%3E%202%3A%20%3Bprint(%221%22)%5D%0A%5B%3Cstandard%3E%202%3A%20%3Bprint(%221%22)%5D%20%20-%3E%20%5B%3Cend%3E%20-1%3A%5D%0A%5B%3Cif%3E%201%3A%20%3B⠀%20if%20(getOption(%22a.test%22)%20%3D%3D%201L)%20⠀%0A⠀%20⠀%20⠀%5D%20n%20-%3E%20%5B%3Cstandard%3E%203%3A%20%3Bprint(%222%22)%5D%0A%5B%3Cstandard%3E%203%3A%20%3Bprint(%222%22)%5D%20%20-%3E%20%5B%3Cend%3E%20-1%3A%5D%0A%5B%3Cend%3E%20-1%3A%5D%20%20-%3E%20%5B%3Creturn%3E%204%3A%5D%0A%5D%0A%5B%3Cstandard%3E%201%3A%20suppressMessages(%7B👇%7D)%5D%20%20-%3E%20%5B%3Creturn%3E%202%3A%5D%0A%0A%0A) :

filename

But then what do we do with : expression() or tryCatch() that might nest several of those in a single call?

expression(if(...) ..., if(...) ...)
tryCatch(if(...) ..., error = function(e) if(...) ...)

Maybe this would be a more general way, so if we have a several nested inputs they would be called 1*, 1** etc:

filename-4

Or the other way around

filename-5

I think I like the latter better because it reads from top to bottom more naturally.

The rule would probably be that whenever control flow is found in an argument the full argument is expanded like this. function definitions might not need special casing.

In few cases when we have deeper nesting of those this might still work, we'd have another box in the old box, and if we use index as in examples above the box indices don't change compared to current situation so this might even be tackled as an ulterior step, so we don't have to dig into the algorithm too much.

Then we'd have to choose if we make it optional, but probably have it work always like this would be fine.

@zkamvar How do these proposals above look ?

zkamvar commented 11 months ago

Thank you for the quick reply! I agree with your assessment and am very much a fan of the latter proposal---the top-down arrangement makes it much easier to read and the box with the starred index gives is just the right amount of context for me to know what's going on.

moodymudskipper commented 11 months ago

Thanks. This has good value I think so should be a priority in this package, but I don't have a lot of time for it these days so it'll have to wait a bit.

Tests cases should be the given example first, then bquote() because it has 2 functions to expand in the same block, and one function that we don't need to expand :

flow::flow_view(bquote)
#> We found function definitions in this code, use the argument nested_fun to inspect them
#> $unquote
#> function(e) {
#>     if (is.pairlist(e)) 
#>         as.pairlist(lapply(e, unquote))
#>     else if (is.call(e)) {
#>         if (is.name(e[[1L]]) && as.character(e[[1]]) == ".") 
#>             eval(e[[2L]], where)
#>         else if (splice) {
#>             if (is.name(e[[1L]]) && as.character(e[[1L]]) == 
#>                 "..") 
#>                 stop("can only splice inside a call", call. = FALSE)
#>             else as.call(unquote.list(e))
#>         }
#>         else as.call(lapply(e, unquote))
#>     }
#>     else e
#> }
#> 
#> $is.splice.macro
#> function(e) is.call(e) && is.name(e[[1L]]) && as.character(e[[1L]]) == 
#>     ".."
#> 
#> $unquote.list
#> function(e) {
#>     p <- Position(is.splice.macro, e, nomatch = NULL)
#>     if (is.null(p)) 
#>         lapply(e, unquote)
#>     else {
#>         n <- length(e)
#>         head <- if (p == 1) 
#>             NULL
#>         else e[1:(p - 1)]
#>         tail <- if (p == n) 
#>             NULL
#>         else e[(p + 1):n]
#>         macro <- e[[p]]
#>         mexp <- eval(macro[[2L]], where)
#>         if (!is.vector(mexp) && !is.expression(mexp)) 
#>             stop("can only splice vectors")
#>         c(lapply(head, unquote), mexp, as.list(unquote.list(tail)))
#>     }
#> }

Created on 2023-07-12 with reprex v2.0.2