Key value field / streamlined mutate block

[christophsax]

Builds on PR #138

• adds a new keyvalue_field (subsequent PRs will improve this)
• uses the keyvalue_field in a mutate block that looks now like normal blocks
• A test for the mutate block covers the essence of PR optional generate_server for fields #138, so the demo fields head_block2 and numeric_field2 are no longer needed and have been removed

[codecov]

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (a985dc9) 78.97% compared to head (b4e2ac9) 81.46%.

Files	Patch %	Lines
R/keyvalue_field.R	94.52%	4 Missing ⚠️
R/server.R	93.33%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #146      +/-   ##
==========================================
+ Coverage   78.97%   81.46%   +2.48%     
==========================================
  Files          17       16       -1     
  Lines        1722     1640      -82     
==========================================
- Hits         1360     1336      -24     
+ Misses        362      304      -58     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[christophsax]

@nbenn Probably preferable to #138

[nbenn]

@christophsax I tried using a keyvalue field as

new_df_block <- function(cols = list(), ...) {

  handle_error <- function(e) {
    warning(e)
    expression()
  }

  parse_one <- function(x) {
    tryCatch(
      parse(text = x),
      error = handle_error
    )
  }

  parse_kv <- function(columns) {

    parsed <- lapply(columns, parse_one)

    if (length(parsed)) {
      parsed <- do.call(c, parsed)
    }

    bquote(
      data.frame(..(exprs)),
      list(exprs = parsed),
      splice = TRUE
    )
  }

  fields <- list(
    columns = new_keyvalue_field(cols),
    expression = new_hidden_field(parse_kv)
  )

  new_block(
    fields = fields,
    expr = quote(.(expression)),
    ...,
    class = c("df_block", "data_block")
  )
}

but the result is not functional. Am I doing something wrong? Can you reproduce?

Screen.Recording.2024-01-05.at.15.31.56.mov

[christophsax]

Ah.. currently only supports transform_block. Bringing this to data_block and plot_block is a copy paste exercise. Can do this afterward.

Or better, a single worker function generate_server_block() with some arguments to control input and output. The methods for tranform_block, data_block and plot_block can call this function with different argument settings? @nbenn

[christophsax]

@nbenn, the second idea from above is done in the last commit. Now your data example should run, too.

[nbenn]

@christophsax does it work for you now? For me it doesn't. I can add/rm fields, but neither is the expression updated nor do I see any tabular output. I believe my parse_kv() function is not executed, but did not do any proper debugging yet. (Might also be my fault). Another observation: in order to see stuff, quite some clicking is necessary in the beginning. Is the UI being initialized properly?

Screen.Recording.2024-01-06.at.09.15.50.mov

[nbenn]

@christophsax starting with a simple example also causes an issue for me:

serve_stack(new_stack(data_block))

does not seem to do anything with the current server architecture.

Screen.Recording.2024-01-06.at.11.31.15.mov

[nbenn]

@christophsax another small experiment is also causing me issues: I tried doing a mutate with two "expression" fields, one for the ... and one for the tidyselect enabled .by argument. The behavior of the "Submit" buttons is a bit funky.

new_mutate_by_block <- function(data, ...) {

  handle_error <- function(e) {
    warning(e)
    expression()
  }

  parse_one <- function(x) {
    tryCatch(
      parse(text = x),
      error = handle_error
    )
  }

  parse_multiple <- function(x) {

    res <- lapply(x, parse_one)

    if (length(res)) {
      res <- do.call(c, res)
    }

    res
  }

  parse_cols <- function(new_cols) {
    parse_multiple(new_cols)
  }

  parse_grouping <- function(group_by) {
    res <- parse_multiple(group_by)
    names(res) <- NULL
    res
  }

  fields <- list(
    new_cols = new_keyvalue_field(value = list()),
    group_by = new_keyvalue_field(value = list()),
    col_expr = new_hidden_field(parse_cols),
    grp_expr = new_hidden_field(parse_grouping)
  )

  expr <- quote(
    dplyr::mutate(..(col_expr), .by = c(..(grp_expr)))
  )

  new_block(
    fields = fields,
    expr = expr,
    ...,
    class = c("mutate_by_block", "transform_block")
  )
}

mutate_by_block <- function(data, ...) {
  initialize_block(new_mutate_by_block(data, ...), data)
}

generate_code_mutate_by_block <- function(x) {
  do.call(
    bquote,
    list(
      attr(x, "expr"),
      where = lapply(x, value),
      splice = TRUE
    )
  )
}

.S3method("generate_code", "mutate_by_block", generate_code_mutate_by_block)

eval_field <- function(x, env) {

  for (cmp in names(x)[lgl_ply(x, is.function)]) {
    fun <- x[[cmp]]
    value(x, cmp) <- do.call(fun, env[methods::formalArgs(fun)])
  }

  x
}

initialize_field_hidden_field <- function(x, env = list()) {
  validate_field(eval_field(x, env))
}

.S3method("initialize_field", "hidden_field", initialize_field_hidden_field)

serve_stack(new_stack(data_block, mutate_by_block))

Screen.Recording.2024-01-06.at.14.18.38.mov

Some observations

• initially the 2nd "Submit" does not do anything
• choosing the first one updates as expected
• on subsequent changes, only the button where the change was made, updates the view
• if several field groups are changed, submit buttons have to be chosen individually

I don't think it's a good idea to have "Submit" functionality at the individual field level. This should be an action that (optionally) is provided by the block and acts on all fields together.

A separate (minor) issue is that we have no "field label" for kv fields which in such a case would be needed to communicate to the user the meaning of these field groups.

(The function eval_field() has to be used for init due to #156. This should be looked into separately.)

[nbenn]

@christophsax I wanted to try introducing a dependency (on another field or the data) and a test-case here could be auto-complete support for columns. Currently this does not seem to be supported (right?). The kv field constructor would need something like an argument for additional auto-complete values (maybe a character vector?) that could be passed to the ace editor.

[christophsax]

@nbenn latest commit fixes the first two issues and adds a test for the first issue.

I don't think the exact look and feel of the keyvalue field options is too important here (I have a follow-up PR, #155, that introduces an option to turn off the submit button). We can easily iterate on that later on and add, e.g., options for autocomplete.

The field label problem can be addressed with the title and description arguments in general field properties (to be introduced in subsequent PRs).

The videos are great, but how about using options(BLOCKR_DEV = TRUE) to save the initial clicks?

(Unclear to me why the GHA workflow does not run anymore)

[nbenn]

I have said my piece.

[christophsax]

@nbenn could you approve or merge? The codecov/project check comes from the fact that server.R has less lines now, due to the out factoring.

[nbenn]

@christophsax I cannot merge either. Feel free to disable the main branch protection under

https://github.com/blockr-org/blockr/settings/branches

and push directly to main. Looking at your diff, I believe that you're trying to re-introduce quite a bit of (removed?) stack server code which therefore probably is untested. Not sure, what exactly your plan is there and whether this is part of your "out factoring".

[DivadNojnarg]

@christophsax: this should be mergeable now

[christophsax]

Thanks! @nbenn, yes, you are right, I missed some stack updates here. Will resolve.

[DivadNojnarg]

Regarding the coverage, we can extract tests from #119 which covers most of the server testing (stack, ...). I opened an issue #170 to do this.

[christophsax]

Seems to be good now. @nbenn is not a big fan of a server logic for fields, but it makes field creation much easier for me. Let's see where we go with this in the long run. Thanks.