Skip to content

Cyurdado update readme - #118

Draft
cyurdadon wants to merge 6 commits into
developfrom
cyurdado-update-readme
Draft

Cyurdado update readme#118
cyurdadon wants to merge 6 commits into
developfrom
cyurdado-update-readme

Conversation

@cyurdadon

Copy link
Copy Markdown

No description provided.

…ng slides 3/4/25, function descriptions in a tabular format for ease of reference and readability
…ides 3/4/25, func format for ease of reference and readability
@kelliemac

Copy link
Copy Markdown
Contributor

thanks so much for doing this, @cyurdadon ! it's looking great. I just did a quick check of the exported package objects (in the man/ folder) to see if all of the functions were covered, and there are a few that aren't currently covered which I think would be good to add mentions of:

  • wilson_ci - I think this is superseded by binom_ci so might be ok to skip, or just make a note that this is the old name under the binom_ci table entry)
  • shorten_git_hash - this is used by get_session_info and can go in the utility functions table
  • exampleData_ICS, exampleData_BAMA, and exampleData_NAb - can we add a section to the README for example datasets?
  • CAVD812_mAB - I think this is also an example dataset?

@kelliemac

kelliemac commented Aug 21, 2025

Copy link
Copy Markdown
Contributor

I also think that in order to make sure this stays up to date in the future, it would be a good idea to add a pull request template for VISCfunctions. We can use the same one we have in VISCtemplates (https://github.com/FredHutch/VISCtemplates/blob/main/.github/pull_request_template.md) and just add a checklist item there nested under "This PR establishes a new function or updates parameters in an existing function" that says something like "The VISCfunctions package README has been updated to reflect this new/updated function"

@kelliemac

Copy link
Copy Markdown
Contributor

one last question to consider: there is some overlap between the package README and the "Overview" vignette. are there any obvious changes we should make to the "Overview" vignette while we are changing the README? I think it's fine to have some redundancy between these two, and I don't think we need to make major changes, but @cyurdadon I am wondering if you looked at the vignette at all and if you have any thoughts on this.

@kelliemac

Copy link
Copy Markdown
Contributor

@slager I see that the statsrv runner is failing - I am guessing that's an issue with packages as we've seen before :( can you take a look?

@slager
slager changed the base branch from main to develop August 21, 2025 21:15
@slager

slager commented Aug 21, 2025

Copy link
Copy Markdown
Contributor

@kelliemac The PR had been created by branching off main instead of develop so it was still using the old CI. I went ahead and fixed it. @cyurdadon et al., please git pull before making any additional commits to this PR.

@slager

slager commented Aug 21, 2025

Copy link
Copy Markdown
Contributor

I think this at-a-glance list of functions is useful. Note that another way to generate a complete set of function documentation is simply to use devtools::build_manual(). The advantage is that this requires no additional maintenance. The manual can also be set up to be organized into groups as desired by using roxygen2 tags.

@cyurdadon

Copy link
Copy Markdown
Author

Hi @kelliemac thanks for your feedback! I havent looked at the Overview vignette but i'll check it out and see if there can be any relevant changes. Thanks @slager , how would we implement that build_manual() function? I'd be curious to see what that looks like - and if its more useful, maybe use that instead?

@cyurdadon

Copy link
Copy Markdown
Author

@kelliemac I added the example datasets to the README as well as the the shorten_git_hash function. I took a look at the Overview RMD and I agree there is a lot of overlap, but Its great as it shows more of example use cases whereas the README has more detailed descriptions on the functions. I also added the PR template.

@kelliemac

Copy link
Copy Markdown
Contributor

thank you so much @cyurdadon for working on this! I wanted to let you know that we haven't forgotten about it, it's just been on hold for the time being.

related to comments above about devtools::build_manual, we asked @slager to create a PR (see #119) to test out using pkgdown tools to automate some of this work of nicely documenting all the functions etc. that are available in the package. the goal of that PR is to create an easy-to-use site that looks something like https://docs.ropensci.org/DataPackageR/reference/index.html but for VISCfunctions; you've probably used sites like this for other R packages before, and personally I find them extremely helpful. they are also easier to maintain vs. manually updating a README file whenever the package changes (we wouldn't want you or someone else to have to be in charge of spending a bunch of time editing this README in the future).

so depending on how that effort goes, we will circle back to this PR and may suggest moving some documentation out of the README and into the roxygen function documentatione. I'd be happy to help with that if we do go that direction... for now, just stay tuned and feel free to check out dave's PR if you want to get a preview of that approach and/or provide any feedback!

@kelliemac

Copy link
Copy Markdown
Contributor

@cyurdadon and @slager, FYI I took a stab at migrating some of this nice VISCfunctions documentation from README.(R)md to the roxygen function documentation so that it will automatically render nicely on the pkgdown site, as discussed above. see #125 and please feel free to make changes directly to that branch (update-docs) as you see fit. so far I've only had time to get that effort started, and I'd be happy to have any help or feedback!

@kelliemac

Copy link
Copy Markdown
Contributor

@cyurdadon and @slager, I just created a few new PRs that attempt to split off some of these suggested changes (and those in #125) into smaller chunks!

now that we have the pkgdown site rendering (https://fredhutch.github.io/VISCfunctions/reference/index.html) I think it's a good time to work on including some of this documentation in the roxygen comments rather than just in the README. @cyurdadon take a look at the pkgdown site and let us know what you think! the "Reference" tab has a list of everything included in the package, so that covers some of what is in these README updates, but I'm sure there are things that aren't yet covered, so maybe we can make a list of which functions need updated documentation?

@kelliemac
kelliemac marked this pull request as draft February 18, 2026 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants