dfe-analytical-services / dfeR

Common R tasks in the Department for Education (DfE)
https://dfe-analytical-services.github.io/dfeR/
GNU General Public License v3.0
9 stars 2 forks source link

Feature/shiny_template_ar_changes #35

Closed adamrobinson361 closed 7 months ago

adamrobinson361 commented 6 years ago

Hi @cwthom - Thanks for the function. Looks like a really good addition!

I've made some changes below to put it in line with the other functions in the package in terms of naming, params and folder structure. Summary as follows:

  1. Renamed to template_app and moved to template.R. Aiming for relevant functions in same script rather than one per script.

  2. Set params to path and type to line up with template_project and got rid of the optionals. Don't think they were really adding anything as they were default.

  3. Function logic again changed a bit to match template_project

  4. Folders match analysis templte apart from Outputs and run scripts as we dont have anything to run. Data, Queries, R, Misc and www.

  5. Moved css to www

  6. Altered globals to reference functions in dfeR that should be helpful e.g. read_sql_script.

  7. Changed the reference to tidyverse to dplyr. Trying to avoid people loading everything in as this becomes a problem when working with packrat as you have to cache all the packages in tidyverse and deps even when not needed (This is very slow).

Let me know if you are happy with those. After that it would be good if you could add some tests similar to the test for the template_project just to check everything works as expected. Similarly i cant get the apps to run so would be good if you could try it out and let me know if you find any probs.

cwthom commented 6 years ago

Hi @adamrobinson361 - thanks for this.

I broadly agree with these but I think it's worth discussing the conventions, particularly around naming things and folder structures. I think the number 1 goal should be to make these functions as friendly and approachable as possible, as we'e intending them to be a used by a wide range of analysts with little or no coding background. So for example:

Appreciate there's a lot of "I think" there! :) But worth trying to sort of conventions relatively early on, so we've got something concrete to stick to.

Let me know what you think. Chris

adamrobinson361 commented 6 years ago

Thanks @cwthom - part of my intention is that we want to set people on the right track to build the skills rather than abstract too much. This is done via a mix of regularly used base, tidy principles and with learning from packaging code. In terms of your points:

  1. We use R folder for functions as you would in packages. The ideal should be functions in there, server calls functions and ui presents the output. Misc is for anything else such as supporting docs. I’d argue the with if you have functional scripts alongside ui server and global that it becomes unclear what the app is. What are you saying you use www for ? That folder is supposed to be for files used in the ui e.g. css. Pngs etc.

  2. I think I agree on descriptive names. Not sure project works though as we don’t actually create an R project. That’s why I went with template. Let’s have a chat about this.

  3. As I said in intro parameters like path are standard and set you up for working in R. We can capture paths in the t training if that helps.

We can catch up offline to get to a place where we’re both happy with this if you’d like.

Adam