search

Biogen-Inc / tidyCDISC

Demo the app here: https://bit.ly/tidyCDISC_app
https://biogen-inc.github.io/tidyCDISC/
GNU Affero General Public License v3.0
108 stars 38 forks source link

Make RECIPES metadata drive #167

Closed Jeff-Thompson12 closed 1 year ago

Jeff-Thompson12 commented 1 year ago

Step 1 on #152

codecov[bot] commented 1 year ago

Codecov Report

Merging #167 (dc990c5) into devel (af3204e) will decrease coverage by 0.57%. The diff coverage is 0.00%.

:exclamation: Current head dc990c5 differs from pull request most recent head fea78c3. Consider uploading reports for the commit fea78c3 to get more accurate results

@@            Coverage Diff             @@
##            devel     #167      +/-   ##
==========================================
- Coverage   19.94%   19.38%   -0.57%     
==========================================
  Files          50       54       +4     
  Lines        4607     4742     +135     
==========================================
  Hits          919      919              
- Misses       3688     3823     +135
Impacted Files Coverage Δ
R/mod_tableGen.R 0.00% <0.00%> (ø)
R/mod_tableGen_fct_filter_adae.R 0.00% <0.00%> (ø)
R/mod_tableGen_fct_filter_adsl.R 0.00% <0.00%> (ø)
R/mod_tableGen_fct_param_opts.R 0.00% <0.00%> (ø)
R/mod_tableGen_fct_recipe_incl.R 0.00% <0.00%> (ø)
R/mod_tableGen_ui.R 0.00% <0.00%> (ø)
R/run_app.R 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Jeff-Thompson12 commented 1 year ago

Hey team. This PR is going to fail the tests at the current moment because the file 'recipes.json' is in the main directory, but I would like some feedback regarding the direction I have taken on making the recipes metadata driven before I invest more time.

The parsing of the recipes file is done by S3 methods. When parsing the JSON file into R. Classes are assigned to either the blocks object and/or an individual block line. I think this approach will allow us to reach our main goal of allowing users to create their own tables and apply their own rules to inclusion and parsing, but would love to hear if you guys agree/disagree.

Jeff-Thompson12 commented 1 year ago

I marked it ready for review, so at least one of you has to give me feedback now.

Jeff-Thompson12 commented 1 year ago

@AARON-CLARK We were experiencing a little bit of this before (mainly with the table title). Part of the problem is that the reactive graph for this module is a bit all over the place. The commit I just pushed seems to have corrected the problem for me. But users may have different experiences depending on the speed of their internet and computer. Not sure how to actually fix the problem without rethinking the reactive graph.

Jeff-Thompson12 commented 1 year ago

@AARON-CLARK I cleaned up the reactive graph a little instead of using debounce(). One problem we were encountering is that the return of the JavaScript run for the recipes was causing the blocks and input$recipe to invalidate before input$COLUMN. (If you watch closely when you select the first recipe in the app as currently constructed, the first table will be built twice. First, without the group and secondly with the group.)

I also cleaned up the pre_ADAE and pre_ADSL reactives a little. There is still much work to be done there.

AARON-CLARK commented 1 year ago

Just converting to draft until current devel branch hits master for cran version 0.2.1

AARON-CLARK commented 1 year ago

If things are taking this long, I think this necessitates the need for a progress bar. What do you think? Should we create an issue to add one?

@Jeff-Thompson12, what do you think about the prospect of adding a progress bar the the table generator? Obviously in a different PR.

AARON-CLARK commented 1 year ago

This is where we are heading. This PR is just step one. I am just setting up the framework to allow us to accomplish this goal.

@Jeff-Thompson12, sounds good. I'll create a specific issue to address this and put it on our roadmap.

Jeff-Thompson12 commented 1 year ago

@Jeff-Thompson12, what do you think about the prospect of adding a progress bar the the table generator? Obviously in a different PR.

I think we might want to go even farther and think about when table generation should be triggered and what UI elements we want the user to be able to interact with while a table is being generated.

Jeff-Thompson12 commented 1 year ago

@AARON-CLARK I more or less took your edits. I have also added in some code chunks to flesh out the "Parsing" section of the vignette.