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

Why functions in helper_function.R are exported? #253

Closed
elong0527 opened this issue Jun 23, 2023 · 8 comments
Closed

Why functions in helper_function.R are exported? #253

elong0527 opened this issue Jun 23, 2023 · 8 comments
Assignees
Labels
question Further information is requested

Comments

@elong0527
Copy link
Collaborator

Could we review if it is necessary to export helper functions?

https://merck.github.io/gsDesign2/reference/index.html#low-level-helpers

@nanxstats
Copy link
Collaborator

Sure, it might be productive to prioritize functions that are not currently used anywhere in code examples and vignettes, as it would require non-trivial changes to the code examples and vignette to un-export previously exported functions.

Or do you mean we need one (or multiple) more accurate file name(s) + section name(s) to organize these functions instead of simply using the generic term "helper"?

@elong0527
Copy link
Collaborator Author

elong0527 commented Jun 23, 2023

Feel those functions should not be exported to user.

For example, why would a user need h1 function.

@elong0527 elong0527 added the question Further information is requested label Jun 23, 2023
@nanxstats
Copy link
Collaborator

nanxstats commented Jun 23, 2023

Sure, that makes sense. My point was, h1() is used in gs_spending_bound() examples in both function documentation and vignettes:

https://merck.github.io/gsDesign2/articles/usage-gs-spending-bound.html
https://merck.github.io/gsDesign2/reference/gs_spending_bound.html

So they need to be rewritten or removed to un-export h1().

@elong0527
Copy link
Collaborator Author

Got it. If that's the only reason to keep them exported. Let me find way to rewrite them.

btw, would you feel vignettes is OK to refer internal function if we did not submit the vignettes to CRAN? (Given some vignettes is to discuss internal logic derivation for technical details)

@nanxstats
Copy link
Collaborator

Got it. If that's the only reason to keep them exported. Let me find way to rewrite them.

btw, would you feel vignettes is OK to refer internal function if we did not submit the vignettes to CRAN? (Given some vignettes is to discuss internal logic derivation for technical details)

Sounds great.

And good point - it might be ok to use ::: in the (article) vignettes because vignettes/articles/ are not even built into the tarball when running R CMD build. So the strict rules from R CMD check will likely not apply to those Rmd files.

@LittleBeannie
Copy link
Collaborator

LittleBeannie commented Jul 14, 2023

Decision: we will keep the following functions exported:
gs_power_npe()
gs_design_npe()
expected_accrual()
ppwe()
s2pwe()

And we can un-export the following functions:
gridpts()
h1()
hupdate()
gs_create_arm()
related PR: #261

@LittleBeannie
Copy link
Collaborator

Let's work on this issue when #261 is merged.

@fb-elong
Copy link

Please review the diff, examples has been udpated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants