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

Example skeleton project - DO NOT MERGE #701

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nikolas-burkoff
Copy link
Contributor

@nikolas-burkoff nikolas-burkoff commented Jul 21, 2022

DRAFT PR not to be merged FyI @gogonzo @pawelru @kumamiao

To use you may need to install teal a few times for RStudio to get it working.

This PR (unmerged) closes insightsengineering/NEST-roadmap#59

We now have:

image
image

image

Or if you chose not to have cdisc:
image

Clearly if we are doing this for real then

  1. I still suggest it goes in teal rather than having to install a new package
  2. The package_skeleton function can be called from other IDEs
  3. We'd want to structure the package_skeleton function in a nicer way and output a proper structure not just a single app.R file
  4. We can add some other options to the UI of creating the project as well e.g. use renv, add folder for custom modules etc.

You could even have options to create specific apps i.e. see insightsengineering/NEST-roadmap#9 (comment) - although they wouldn't live in teal itself though could live in teal.gallery @dinakar29

@nikolas-burkoff nikolas-burkoff changed the title Example skeleton project Example skeleton project - DO NOT MERGE Jul 21, 2022
@cicdguy
Copy link
Contributor

cicdguy commented Jul 21, 2022

This looks great, @nikolas-burkoff!

You could even have options to create specific apps

Skeleton apps derived from sample apps on teal.gallery sounds like a great idea. We should consider that in the near future. Let me add that as an IDR task, as it seems pretty straightforward.

Icon: teal.png

Parameter: use_cdisc
Widget: CheckboxInput
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(in the future) Can the Widget be for example?

Widget: availableModulesInput

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can only have the following:

image

We may want an addin to do the add modules part rather than at this project creation time. And for creating your own custom modules you could maybe? use snippets

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is snippet a Rstudio-exclusive feature?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think snippet is RStudio exclusive feature (i.e. you can add code snippet on vscode too). However, the syntax will different.

@pawelru
Copy link
Contributor

pawelru commented Jul 21, 2022

The addon looks good but the content is not there yet (however it's a draft so not sure if I can focus on it at all). For sure I would go away from single-file apps in favour of more sophisticated multiple files. The content is to-be-decided (researched). Feel free to to have a look what's in already archived tealApp project/repo somewhere.
I also think that it would be better/easier to have scripts (in inst/) as files than scripts as character string.

I like addon because it promotes good practice of using "New Project" (at least from RStudio).
I dislike addon as the configuration possibilities is very limited. I can imagine that the number of config controls would grow (e.g. use testing, use logging and if yes - what log type or even custom module selector as pointed by Dawid) and we might want to group them, make it optional / conditional, etc. This is not possible. I am not yet sure if that's bad - maybe it's good to have it super simple - but I see it as a limitation for the future.

@gogonzo
Copy link
Contributor

gogonzo commented Aug 1, 2022

@kumamiao @donyunardi we have not decided exactly what is needed here. If addin should be a single file or multiple files.
There was proposition of having data.R, modules.R sourced from the app.R but wasn't formally confirmed. We suppose to promote good practices but I don't know if these are good practices. Also, we probably should initialize with empty www/css, www/js. Am I missing something?

Additionally, I think that it shouldn't be a separate package because users shouldn't be bothered to install a new package from github. I think we should rather keep it in teal. Alternatives are not good enough, because if we make a new independent package hosted on github then users will be requested to install this. Package will not be able to be installed on CRAN because it's a utility package, which means it can't be in teal/Suggests.

@donyunardi
Copy link
Contributor

donyunardi commented Aug 1, 2022

@nikolas-burkoff mentioned this earlier on creating a custom_modules folder, which I thought was a good idea. Another one that comes to mind is to create a folder to store custom functions.

I think separating and sourcing data.R and modules.R from app.R is a good idea. This will make the code more organized and easier to debug. And, while we're on this topic, when I looked around in Roche GitHub, I noticed people's modules= code was long due to the number of parameters for each module and this makes it hard to see the organization of the modules.

What do you think if we separate the module call into its own file under a separate folder?
Here's to illustrate what I meant:

app.R

library(teal)

source("data.R")
source("modules.R")
lapply(list.files("module", full.names = TRUE), source)

app <- init(
  data = data,
  modules = modules,
  header = "My first teal application"
)

if (interactive()) {
  runApp(app)
}

data.R

data <- teal_data(
  dataset("IRIS", iris),
  dataset("MTCARS", mtcars)
)

modules.R

modules <- modules(
  x1,
  x2,
  modules(
    label = "Nested",
    x1,
    x2
  )
)

module/module1.R

x1 <- example_module()

module/module_2.R

x2 <- example_module()

The structure would look like this:

/
-- app.R
-- data.R
-- modules.R
-- module/
    -- module1.R
    -- module2.R

@nikolas-burkoff
Copy link
Contributor Author

Additionally, I think that it shouldn't be a separate package because users shouldn't be bothered to install a new package from github. I think we should rather keep it in teal.

Yes I agree

on creating a custom_modules folder, which I thought was a good idea

So my hope was that existing modules would soon be simplified enough to not need separate files for all of them (i.e. definitions.R -> which stores all the data extract specs etc. and then one file for the app). The custom_modules folder was originally meant to be used if your app used user-defined modules (i.e. a module created yourself not one from tmg, tmc etc.) but I'm open to whatever structure we think makes sense.

@donyunardi
Copy link
Contributor

donyunardi commented Aug 11, 2022

So my hope was that existing modules would soon be simplified enough to not need separate files for all of them (i.e. definitions.R)

Sure, that sounds good too.

This skeleton template does seem like a possible solution for the marketplace. However, the template won't be able to show module visualization for the user to see before making a selection. The initial proposal was for the user to be able to see and select the module(s) from the marketplace. I am not sure if that requirement can be fulfilled using the skeleton template.

It feels like we're still undecided between the marketplace will produce one file vs multiple files. I think multiple files will improve code readability and maintenance, but this could lead to major changes to our vignettes. Would this be a good topic for refinement?

@gogonzo gogonzo marked this pull request as ready for review August 31, 2022 10:24
@gogonzo gogonzo marked this pull request as draft August 31, 2022 10:24
@github-actions
Copy link
Contributor

badge

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  -------------------------------------------
R/default_filter.R                   7       7  0.00%    17-27
R/dummy_functions.R                 74      61  17.57%   12-95
R/example_module.R                  17      17  0.00%    19-35
R/get_rcode_utils.R                 52       2  96.15%   94, 99
R/get_rcode.R                      131      51  61.07%   66, 73, 78, 139-148, 186, 212-261
R/include_css_js.R                  24       0  100.00%
R/init.R                            39      21  46.15%   171, 182-183, 236-257
R/log_app_usage.R                   38      38  0.00%    34-119
R/logging.R                         13      13  0.00%    11-28
R/module_nested_tabs.R             113      16  85.84%   57, 96, 100-117, 163, 212, 257
R/module_tabs_with_filters.R        65       0  100.00%
R/module_teal_with_splash.R         33       2  93.94%   62, 74
R/module_teal.R                    119      20  83.19%   49, 52, 142-143, 156-162, 168-174, 197, 227
R/modules_debugging.R               19      19  0.00%    41-60
R/modules.R                        115       9  92.17%   218, 423-448
R/project_skeleton.R                34      34  0.00%    13-60
R/reporter_previewer_module.R       12       2  83.33%   18, 22
R/show_rcode_modal.R                20      20  0.00%    17-38
R/utils.R                            6       0  100.00%
R/validations.R                     62      39  37.10%   103-355
R/zzz.R                             11       7  36.36%   3-14
TOTAL                             1004     378  62.35%

Results for commit: 94c2a21

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

Unit Tests Summary

    1 files    10 suites   11s ⏱️
107 tests 107 ✔️ 0 💤 0
207 runs  207 ✔️ 0 💤 0

Results for commit b66d22f.

@kpagacz
Copy link
Contributor

kpagacz commented Sep 19, 2022

Oh god, it's happening

@asbates
Copy link
Contributor

asbates commented Dec 15, 2022

Came across this so thought I'd share my 2 cents. It may have already been done but I'd highly suggest looking at golem and rhino because app templates are a big part of both. They also take very different approaches so it could be useful to compare and contrast.

@pawelru
Copy link
Contributor

pawelru commented Dec 15, 2022

@asbates please see linked tasks to insightsengineering/NEST-roadmap#9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Research Rstudio tools for teal skeleton
7 participants