Adding step ribbon function in add_CI()

[ddsjoberg]

What changes are proposed in this pull request?

• Added a step ribbon function to add_CI().

library(visR)

adtte %>%
  estimate_KM(strata = "TRTP") %>%
  visr() %>%
  add_CI()

^{Created on 2022-07-11 by the reprex package (v2.0.1)}

Did you include unit tests for the proposed change/bug fix (https://testthat.r-lib.org/)?
Unit tests are included

If there is an GitHub issue associated with this pull request, please provide link.
closes #319

---

Checklist for PR reviewer

• PR branch has pulled the most recent updates from main branch. Ensure the pull request branch and your local version match and both have the latest updates from the main branch.
• If a new function was added, function should be included in _pkgdown.yml
• If a bug was fixed, a unit test was added for the bug check
• Run pkgdown::build_site(). Check the R console for errors, and review the rendered website.
• Code coverage is suitable for any new functions/features. Review coverage with withr::with_envvar(new = c("NOT_CRAN" = "true"), covr::report()). Before you run, begin a fresh R session without any packages loaded.
• R CMD Check runs without errors, warnings, and notes
• usethis::use_spell_check() runs with no spelling errors in documentation
• Has NEWS.md been updated with the changes from this pull request under the heading indicating the latest version. If there is an issue associated with the pull request, reference it in parentheses at the end update (see NEWS.md for examples).
• Has the version number been incremented using usethis::use_version(which = "dev")
• Run devtools::build_readme() to build the README.md file.
• Approve Pull Request
• Merge the PR. Please use "Squash and merge".

[SHAESEN2]

Looks good but please ensure full coverage with UAT where you describe the requirements for these functions and you test them.

[ddsjoberg]

Looks good but please ensure full coverage with UAT where you describe the requirements for these functions and you test them.

Thanks for the quick review @SHAESEN2 !

I was waiting to add unit tests until after the other PR was merged, so I could also use snapshot testing. I've now added the unit tests including some vdiffr checks.

[SHAESEN2]

Almost there. I would still like to see more explanation as to why we would use these functions in the documentation.

[ddsjoberg]

Almost there. I would still like to see more explanation as to why we would use these functions in the documentation.

I added a sentence. You can add more detail if you'd like to the documentation file.

[SHAESEN2]

Please add rationale for function and make clear what you are testing.

[ddsjoberg]

Please add rationale for function and make clear what you are testing.

Thanks for reviewing @SHAESEN2 ! We've resolved everything except a potential minor documentation and text in the testthat TOC. Those can be updated anytime in the future.

I'll try to address your comments for prosperity, but I truly do not think I understand what you're looking for.

Thanks for the explanation but the user still does not know why he would choose this option instead of what the package provides by default. Could you add some rationale for this function?

You're asking for a rationale why this function exists at all (which I think is pretty obvious), or why do we have a stat version and then also the ability to wrap our stat inside ggplot2::geom_ribbon()? ggplot functions most often come in geom and stat pairs (e.g. stat_smooth, geom_smooth). The reasons for this are complex and you can read more about it in the ggplot vignette "Extending ggplot2" (writing these extensions is hard!).

Still dont understand what this test is doing. Perhaps you can resolve this with Mark during my time off.

At the end of the day, it's a test that makes sure the function works with strata and without strata (but that is language specific to a KM plot and this is a general function). I did a visual inspection of the produced figure and saved the snapshot test.

[bailliem]

I would propose to merge and then work on the test / document updates in a seperate issue. @SHAESEN2 and @ddsjoberg