Gilead-BioStats / gsm

Good Statistical Monitoring R Package
https://gilead-biostats.github.io/gsm/
Apache License 2.0
36 stars 9 forks source link

Implement a glossary #1667

Closed jonthegeek closed 1 month ago

jonthegeek commented 1 month ago

Overview

Glossary added as man/glossary. Definitions are reusable for things such as function parameters or vignettes.

Closes #1658.

Test Notes/Sample Code

Notes:

@jwildfire I played with this a little, and have it mostly solved for a reusable glossary. Info from R Packages and roxygen2 help. However, I tried specifying the link like they show there, and it always applies (not selectively like the help implies), thus the links to other functions were broken. I'll dig into it a bit more + see if they have advice tomorrow. I suspect it's a bug, possibly specifically in the context of @param.

jonthegeek commented 1 month ago

Sadly it does appear to be a bug, which I've reported to the roxygen2 team. I'm going to move forward with the idea anyway, and assume that we'll be able to fix the handful of files one way or another. For now I'll write them in the manner that will work for documentation, and then we can later make them ALSO work for vignettes when/if they address my issue.

jonthegeek commented 1 month ago

@jwildfire @lauramaxwell I think I like this general idea a lot! Things to check before I push forward:

Note that, if we decide to change the definitions, we only have to change them in one place (so no big deal)... but I plan to do this for all of our reused parameters, so we should probably figure out the general idea to minimize re-working.

What do you think?

jonthegeek commented 1 month ago

I think this is actually "done enough" for #1658. Applying the glossary will be at least one push as part of #1628, but this specific ticket shouldn't hold off until every parameter is re-documented. Let's get this one sorted out as its own PR, and THEN we can decide how to systematically apply it.

jonthegeek commented 1 month ago

I think this will be helpful for centralizing the documentation for df inputs and a few other unambiguous params, like vThreshold. One thing i'm finding through reviewing this is that we use the same param name for things across different functions that aren't identical, such as `strGroupColandstrType. Not sure that this a major issue, just wanted to bring it up before we go too far down this path. It seems the use for the glossary will be limited to under ten params, but centralization will still ultimately probably be super helpful.

Check out discussion #1682 for something related to this. You're definitely right, though; sometimes it makes sense to use the same name for at least slightly different meanings.

I have one more idea to experiment with for cases where there's a shared core, but possibly slightly different specifics. {roxygen2} doesn't allow you to @importParams and enhance, but I have an idea for how to make that work (so, for example, in the dfTransformed we could use a shared definition that has all the generic information, and then tack on a bit about which function we expect you to use to make it for this function).

I don't want to force unnatural rigidity or anything, but it's so easy for definitions to drift apart if we aren't vigilant, and that can be really confusing as a user. Plus it's tedious to have to describe the same basics about a parameter that we've described before.

jonthegeek commented 1 month ago

@lauramaxwell See what you think about this updated idea. I feel like the two functions I define in aaa-shared.R could be split off to form the heart of a fairly useful roxygen2-add-on package (in which case the calls would be something like `r glossygen::gloss_param("dfMetrics")` instead of `r gloss_param("dfMetrics")`). I might add a function or two to quickly create the yaml/Rmd helper files, too, to make the process as easy as possible (and if I ever make the package, I'd probably also add things to parse your R folder and guide you through usage).

I'd like to continue playing with this to see what's easy to use/helpful. If this makes sense to you, though, we could go ahead and merge this one, and then expand on it in future PRs.