The Wayback Machine - https://web.archive.org/web/20201001050540/https://github.com/plotly/dashR/issues/88
Skip to content

/ dashR

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

Jump to bottom

Revise assert_valid_callbacks to permit passing unnamed lists as callback output #88

Open
z267xu opened this issue May 14, 2019 · 1 comment
Open

Revise assert_valid_callbacks to permit passing unnamed lists as callback output #88

z267xu opened this issue May 14, 2019 · 1 comment
Labels
bug

Comments

@z267xu
Copy link

@z267xu z267xu commented May 14, 2019 •
edited

In Python, we may not need to give names to a list. In R, we would better do so, or the extraction may return NULL. The output is a list with first argument id and second property. Hence, if the users fail to give names, the following line would throw an error

dashR/R/utils.R

Lines 360 to 363 in 6bc44b5

# Assert that the component ID as passed is a string.
if(!(is.character(output$id) & !grepl("^\\s*$", output$id) & !grepl("\\.", output$id))) {
stop(sprintf("Callback IDs must be (non-empty) character strings that do not contain one or more dots/periods. Please verify that the component ID is valid."), call. = FALSE)
}

So, we can add an outputCheck function bellow.

outputCheck <- function(output) {
  
  if(length(output) != 2) {
    
    stop(sprintf("Both 'id' and 'property' should be provided"), call. = FALSE)
    
  } else {
    
    namesOutput <- names(output)
    
    if(is.null(namesOutput)) {
      
      warning(sprintf("output names are missing. The first element in output would be set as 'id' and the second would be 'property'"), call. = FALSE)
      
      names(output) <- c("id", "property")
      
    } else {
      
      if(!all(namesOutput %in% c("id", "property"))) {
        
        warning(sprintf("One of output names is missing."), call. = FALSE)
        
        names(output) <- if(all(namesOutput == c("", "property")) | all(namesOutput == c("id", ""))) {

          c("id", "property") 
          
        } else if(all(namesOutput == c("property", "")) | all(namesOutput == c("", "id"))) {
          
          c("property", "id")
          
        } else stop(sprintf("Only 'id' and 'property' can be provided"))
        
      } else if (all(namesOutput %in% "id")) {
        
        warning(sprintf("Both names are 'id', the second one would be set as 'property'"))
        
        names(output) <- c("id", "property")
        
      } else if (all(namesOutput %in% "property")) {
        
        warning(sprintf("Both names are 'property', the first one would be set as 'id'"))
        
        names(output) <- c("id", "property")
        
      } else NULL
    }
  }
  
  output
}

and draw this line

output <- outputCheck(output)

in

dashR/R/dash.R

Lines 438 to 444 in 6bc44b5

callback = function(output, params, func) {
assert_valid_callbacks(output, params, func)
inputs <- params[vapply(params, function(x) 'input' %in% attr(x, "class"), FUN.VALUE=logical(1))]
state <- params[vapply(params, function(x) 'state' %in% attr(x, "class"), FUN.VALUE=logical(1))]
# register the callback_map

before assert_valid_callbacks(output, params, func)

It has been tested and should handle all kinds of missing given names
@rpkyle
@rpkyle rpkyle self-assigned this May 14, 2019
@rpkyle rpkyle added the bug label May 14, 2019
@rpkyle
Copy link
Member

@rpkyle rpkyle commented May 14, 2019 •
edited

Thanks @z267xu -- nice find and great first issue!

This is definitely what I would call a bug, since we want to permit app developers to use similar shorthand in DashR as in Dash for Python. For example, either of the following is acceptable callback syntax:

@app.callback(
    Output(component_id='my-div', component_property='children'),

@app.callback(
    Output('my-div', 'children'),

Given the way assert_valid_callbacks is currently written, the following line fails in DashR:

app$callback(output = list("my-div", "children"),

with the error

Error in if (!(is.character(output$id) & !grepl("^\\s*$", output$id) &  : 
  argument is of length zero

The reason for this is that the corresponding test in assert_valid_callbacks expects a named list for output, which is actually not idiomatic for Dash:

dashR/R/utils.R

Lines 360 to 363 in 6bc44b5

# Assert that the component ID as passed is a string.
if(!(is.character(output$id) & !grepl("^\\s*$", output$id) & !grepl("\\.", output$id))) {
stop(sprintf("Callback IDs must be (non-empty) character strings that do not contain one or more dots/periods. Please verify that the component ID is valid."), call. = FALSE)
}

I believe this test should actually (and in this order)

1️⃣ assert that the argument passed to output is a list
2️⃣ assert that this list has two, and only two, elements
3️⃣ treat output[[1]] as id, and output[[2]] as its property, them perform same check as before

I'm not opposed to writing additional tests to inspect the output, but I'd rather keep it simple whenever possible.

@alexcjohnson
@rpkyle rpkyle changed the title default names for `output` Revise assert_valid_callbacks to permit passed unnamed lists as callback output May 14, 2019
@rpkyle rpkyle changed the title Revise assert_valid_callbacks to permit passed unnamed lists as callback output Revise assert_valid_callbacks to permit passing unnamed lists as callback output May 14, 2019
@rpkyle rpkyle removed the high priority label Aug 26, 2020
@rpkyle rpkyle removed their assignment Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug
2 participants
@rpkyle @z267xu
You can’t perform that action at this time.