Skip to content
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

Fix modules to use teal_data instead of tdata #588

Closed
15 tasks done
gogonzo opened this issue Oct 18, 2023 · 1 comment · Fixed by #603
Closed
15 tasks done

Fix modules to use teal_data instead of tdata #588

gogonzo opened this issue Oct 18, 2023 · 1 comment · Fixed by #603
Labels

Comments

@gogonzo
Copy link
Contributor

gogonzo commented Oct 18, 2023

part of insightsengineering/teal#937

installation instructions:

staged.dependencies::dependency_table(
  project = "insightsengineering/teal.modules.general@https://github.com",
  project_type = "repo@host",
  ref = "refactor",
  verbose = 1
) |>
staged.dependencies::install_deps()

Steps:

  1. Assign yourself by writing your name next to the module in "Progress list" below.
  2. Open separate branch tm_module_of_your_choice@refactor.
  3. Make changes relevant changes. See model example:
  • fix asserts
# from
checkmate::assert_class(data, "tdata")
  
# to
checkmate::assert_class(data, "reactive")
checkmate::assert_class(isolate(data()), "teal_data")
  • Fix call to merge_expression_module, data_extract_multiple_srv and merge_expression_srv by removing join_keys argument. I adjusted teal.transform functions to work with new data (reactive teal_data). Adjusted methods doesn't need join_keys object as join_keys are already in teal_data object.
  • Fix merge call evaluation in the module. Object data inherits from qenv so we don't need to convert tdata -> qenv anymore. Beware, exact calls in your module might differ from the one below.
# from 
anl_merged_q <- reactive({	   
  req(anl_merged_input())
  teal.code::new_qenv(tdata2env(data), code = get_code_tdata(data)) %>% 
      teal.code::eval_code(as.expression(anl_merged_input()$expr))
})
# to
anl_merged_q <- reactive({
  req(anl_merged_input())
  data() %>%
    teal.code::eval_code(as.expression(anl_merged_input()$expr))
})
  • there might be more calls to data. Please beware that now data is a reactive holding teal_data instead of tdata (list of reactives).
  • example change in validation rules
iv$add_rule("variables", shinyvalidate::sv_in_set(
 # set = names(data[[dataname]]()), message_fmt = "Not all selected variables exist in the data" # old
 set = names(data()[[dataname]]), message_fmt = "Not all selected variables exist in the data" # new
))
  1. Make PR to the @refactor branch with only one module changed. ONE PR, ONE MODULE. Include example app in the initial comment!
  2. Add a link to the PR next to the "Progress list"

Progress list:

@gogonzo
Copy link
Contributor Author

gogonzo commented Nov 23, 2023

@donyunardi

First package is refactored. Went pretty easy. There are some validate/req artifacts in some modules which appears for a fraction of seconds. We will take care of it later 💪

@gogonzo gogonzo closed this as completed Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant