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

create_project() #71

Merged
merged 51 commits into from
Aug 6, 2024
Merged

create_project() #71

merged 51 commits into from
Aug 6, 2024

Conversation

JT-39
Copy link
Collaborator

@JT-39 JT-39 commented Jun 11, 2024

Brief overview of changes

Two main files have been added to create the new function create_project().

  • R/create_project.R
  • inst/rstudio/templates/project/create_project.dcf

The first includes the function to create and populate a new R project. It includes several files and folders by default such as _analysis/analysis.qmd, a renv.lock and tests/. There are options that allow one to alter the structure of the project (for example for package development or a publication pipeline).

The second file provides the binding to create a GUI option to create the R project. This appear when one clicks the "New project" at the top right of the RStudio IDE.

Why are these changes being made?

These changes are being made as the solution for the issue - Pre-populated project template #70

Detailed description of changes

  • R/create_project.R added which is the main function to create the pre-populated project.
  • inst/rstudio/templates/project/create_project.dcf which builds the GUI to start the project.
  • Frederick logo added to above folder so it appears in the GUI.
  • Amended the Imports and Suggest in the DESCRIPTION file to facilitate create_project().
  • Test of basic functionality - does it create the folders and files we expect.
  • Test to see if the warning appears if {renv} is not installed.
  • devtools::check() has been re-run, creating the create_project() vignette.
  • The _pkgdown.yml has also been updated to include the new function.

Additional information for reviewers

A set of tests have been added, however these are not exhaustive and could include more.

Currently the init_renv parameter of the function is set to TRUE as default. This initiates {renv} in the created project, which is best practice. However, the renv.lock is setup to record several packages ({rmarkdown}, {testthat}, {renv}, {stringr}). There is a minor issue that these packages are not installed in the new project and so the message appears:

"- None of the packages recorded in the lockfile are currently installed."

This can be rectified by executing renv::restore() which will install those packages. These packages are needed to create the tests and run the Quarto file. They could be reviewed. Potentially the init_renv is set to FALSE so it is more optional but I think promoting {renv} is best practice.

More documentation will be necessary to describe and explain how the function work, especially the above.

The actual structure of the project created should also be reviewed, especially for the publication project pipeline option (I have just guessed this and assume there is an existing project template which would go better there).

Issue ticket number/s and link

Pre-populated project template #70

@JT-39 JT-39 added enhancement New feature or request help wanted Extra attention is needed labels Jun 11, 2024
@JT-39 JT-39 self-assigned this Jun 11, 2024
@JT-39 JT-39 linked an issue Jun 11, 2024 that may be closed by this pull request
@chfoster
Copy link

Don't think this error message is necessarily right:
image

  • my REPOS folder isn't a project, and also why 3 options but 1 and 3 both mean no? 😂

@JT-39
Copy link
Collaborator Author

JT-39 commented Jul 29, 2024

Hey @JT-39 - thanks for raising this, this is absolutely fantastic! Love the GUI part of it!

Have left a few specific comments on the code, and have a couple of more general thoughts too:

  1. Is there a main analysis script that you're expecting people will run? (thinking of something like a run_all.R in the root or R directories)
  2. This could be a prime time to look at adding the praise or other equivalent package from Find a way to make use of the praise package #61, could add something fun into the Rprofile as a DfE specific greeting
  3. Have you considered a separate vignette to walk through how to download the package and then use the GUI to create a new project? Thinking of taking it right to the very basics for someone still unfamiliar with R. This could be done after this PR if too much work to add here.

Thanks! To answer your points:

  1. Not really at the moment. I think there should be, but unsure what to call it/how it fits in with the 02_analysis/ folder. Does it replace it? Should it be a .qmd or just plain .R script. Name it main?

  2. I like this idea! I've added a .Rprofile and used .First <- function() {message('Welcome to your dfeR project!')}. I tried adding {praise} but it didn't like the package coming from git with {renv}. Also, when entering the project for first time the user is a bit blasted from {renv} about installing packages from the renv.lock file. Should discuss.
    EDIT: {praise} now working. Issue was I had the dev package installed from github. Have also updated the renv stuff so its a bit more secure and deletes the project if it doesn't initialise properly. Still being verbose though.

  3. Yes, I will do this. Another good idea. Will explore how to do this!

@cjrace cjrace linked an issue Aug 5, 2024 that may be closed by this pull request
@cjrace
Copy link
Contributor

cjrace commented Aug 5, 2024

All looks good @JT-39, happy for this to get merged in once a couple of small last bits have been tidied:

  1. Now you've added emo, the package won't install for the first time as it can't find it. Good news is I think this is easy to fix, as emo is a GitHub repo you need to add a new line with Remotes: hadley/emo into the DESCRIPTION file so that it knows to look at GitHub to install it's dependencies too.
  2. You should add yourself as a contributor in the DESCRIPTION 🎉 copy how Jen did her line if you want an example of how to do it
  3. I'd recommend running usethis::use_tidy_description() after 1. and 2. to reorder and format the DESCRIPTION file neatly too
  4. There's conflicts in the inst/WORDLIST file, from a glance it looks like you've added 'center' in but the way it remakes the list has reordered it and caused the conflict. Assuming you'll be able to resolve this pretty easily / quickly. Resolving this will also then unblock all the CI checks on this PR (I've ran checks locally and it seems to pass fine so not expecting any issues with those)
  5. As this is pretty much ready to go, do you want to have a go at bumping the package up a minor version as this is a nice new feature. So make it v0.4.0 using the notes in the contributing file for how to do this with usethis and add a quick note in the space it'll give you in the NEWS.md file about the new function.

Once the above is done, assuming the CI checks all pass I'll approve, and once merged I'll release the GitHub version to match.

I've also raised #76 to cover the vignette separately, if that goes in before anything else, it can go in as a patch update so would then be v0.4.1 (though we can agree this on the PR at the time it's ready)

@JT-39
Copy link
Collaborator Author

JT-39 commented Aug 6, 2024

All looks good @JT-39, happy for this to get merged in once a couple of small last bits have been tidied:

  1. Now you've added emo, the package won't install for the first time as it can't find it. Good news is I think this is easy to fix, as emo is a GitHub repo you need to add a new line with Remotes: hadley/emo into the DESCRIPTION file so that it knows to look at GitHub to install it's dependencies too.
  2. You should add yourself as a contributor in the DESCRIPTION 🎉 copy how Jen did her line if you want an example of how to do it
  3. I'd recommend running usethis::use_tidy_description() after 1. and 2. to reorder and format the DESCRIPTION file neatly too
  4. There's conflicts in the inst/WORDLIST file, from a glance it looks like you've added 'center' in but the way it remakes the list has reordered it and caused the conflict. Assuming you'll be able to resolve this pretty easily / quickly. Resolving this will also then unblock all the CI checks on this PR (I've ran checks locally and it seems to pass fine so not expecting any issues with those)
  5. As this is pretty much ready to go, do you want to have a go at bumping the package up a minor version as this is a nice new feature. So make it v0.4.0 using the notes in the contributing file for how to do this with usethis and add a quick note in the space it'll give you in the NEWS.md file about the new function.

Once the above is done, assuming the CI checks all pass I'll approve, and once merged I'll release the GitHub version to match.

I've also raised #76 to cover the vignette separately, if that goes in before anything else, it can go in as a patch update so would then be v0.4.1 (though we can agree this on the PR at the time it's ready)

Think this has all been done now! 😀

There are conflicts but from the update of the DESCRIPTION and NEWS file due to the increment in version, assume this is fine?

Copy link
Contributor

@cjrace cjrace left a comment

Choose a reason for hiding this comment

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

Happy to approve! Raised #77 to look more into the linting as it shouldn't affect usage so we can deal with it separately

@cjrace cjrace merged commit 0ae2aba into main Aug 6, 2024
7 of 8 checks passed
@cjrace cjrace deleted the create-proj-func branch August 6, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre-populated project template Find a way to make use of the praise package
3 participants