aim-rsf / browseMetadata

An R package to help a researcher browse metadata for health datasets and categorise variables based on research domains
https://aim-rsf.github.io/browseMetadata/
GNU General Public License v3.0
3 stars 1 forks source link

Change installation instruction of the pkg #43

Closed BatoolMM closed 8 months ago

BatoolMM commented 8 months ago

This is to fix #26

BatoolMM commented 8 months ago

It is ready for review - I assume that the new pkg instructions won't work if the repo is private.

I tried to make sure I captured all functions and packages you used - one thing I wasn’t sure about it is insight pkg - @RayStick where did you use it? I see that you call it but not sure where it was used inside the function.

RayStick commented 8 months ago

Thanks @BatoolMM - these are some good improvements! I want to review this PR properly - will probably look later this afternoon or tomorrow.

BatoolMM commented 8 months ago

No rush - take your time.

RayStick commented 8 months ago

@BatoolMM I incorporated my latest updates to the README via a merge (instead of a rebase) - hope that's okay! Not sure it's the best option but it worked i.e. the changes I made to the README are now incorporated into this branch alongside the changes you have made

BatoolMM commented 8 months ago

Oh, thank you!

RayStick commented 8 months ago

I tried to make sure I captured all functions and packages you used - one thing I wasn’t sure about it is insight pkg - @RayStick where did you use it? I see that you call it but not sure where it was used inside the function.

Thanks @BatoolMM, the insight package was for the print_colour functions, but actually your comment reminded me that I wanted to changed this. I just made some changes in my last commit. So I am no longer using the insight package but I am now using the cli package. Can you please add this as a dependency in the places you added the others? Thanks!

BatoolMM commented 8 months ago

Of course, I will add it now!

BatoolMM commented 8 months ago

I don't add the dependencies manually, I run the command and they get added in the appropriate files - NAMESPACE shouldn't be changed manually at all. This is one of my favourite resources when working with R packages: https://r-pkgs.org/workflow101.html.

BatoolMM commented 8 months ago

To break it down and make it easier to grasp the changes in this PR, let me share how I handle dependencies (note, this is my approach but doesn't mean it's the only or best way or even correct).

For normal R scripts that aren't meant for building a package, I install and load packages the typical way:

install.packages("pkg")
library(pkg)

But when working on an R package, I take a different approach because it's crucial that users can access all needed dependencies, no matter what's already in their environment. Quick tip: DESCRIPTION and NAMESPACE are key files here. The DESCRIPTION file is your package's ID – it names your package and lists its dependencies. Any package listed there under 'Imports' gets installed along with your package. Meanwhile, the NAMESPACE file tells which functions are ready to be used (loaded) so you don't have to load it with library(). So, when I need to add a package as a dependency, I head to these files.

Suppose I want to use a function called fun from pkgA. I'd do this:

usethis::use_package("pkgA") 
# This adds the package to your DESCRIPTION automatically. 
# It means it'll get installed with your package, saving users the hassle.

Next, let's talk about how to load the package or specific functions from it into the user's environment, not just install them. This can be handled in a file set up for package-level documentation. It's a dummy file located at R/{pkgname}-package.R and might start like this:

#' @keywords internal 
"_PACKAGE"

You don;t generate it manually but by running:

usethis::use_package_doc()

Once generated, you can specify functions to be loaded from specific packages. This means these functions will be available without needing library() or ::.

usethis::use_import_from(package = "pkgA", fun = "fun")

Or you might add this manually if needed:

#' @import pkgA pkgB 
#' @importFrom pkgC fun

After adding the functions or packages you want to your script, make sure to run:

devtools::document()

This will update the NAMESPACE, and you're good to go.

Hope that clears things up! For all the nitty-gritty details, check out here and here.

RayStick commented 8 months ago

Hi Batool Thanks for the super helpful explanation!

Also thanks for changing the title to

Title: Browses available metadata, to catergorise/label each variable in a dataset.

Can we also change the description to this below. Is it too long or okay?

Description: This package currently contains one function, domain_mapping. This function takes two inputs: a metadata file (that has been downloaded in json format from https://modelcatalogue.cs.ox.ac.uk/hdruk_live) and a csv file (created by the user, that lists research domains of interest). The function will read in the metadata file for a chosen dataset, loop through all the variables, and ask the user to catergorise/label each variable as belonging to one or more domains. The domains will appear in the Plots tab and information about the dataset will be printed to the R console, for the user's reference in making these categorisations. A log file will be saved with the catergorisations made. To speed up this process, some auto-categorisations will be made by the function for commonly occurring variables; these should be verified by the user by checking the csv log file. Example inputs are provided within the package data, for the user to run this function in a demo mode.

It was getting a little confusing with all the threads I made. Do you want to confirm that the title/description is changed in all the places it needs to be? Let me know when it is, then I can take a fresh look at everything. I think we can merge it soon =D

BatoolMM commented 8 months ago

Hi Rachael, I think it's been changed - but let me run a few checks, to make sure all files are updated with these changes.

BatoolMM commented 8 months ago

Hi @RayStick , I have checked all changes but I had to remove dashes and special characters (and URL) because it gave me Malformed YAML. Can you review the DESCRIPTION and make sure it make sense with all changes, pls?

BatoolMM commented 8 months ago

To me, this is looking pretty good! Happy to merge if you are? We can then think about testing the installation of the package, but (as you say) we may have to just make it public in order to test that properly.

This sounds fantastic, feel free to merge it.

BatoolMM commented 8 months ago

Thank you so much for merging!