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

Import all functions once in R/gsDesign2-package.R, and remove the pkg:: qualifiers #464

Merged
merged 5 commits into from
Sep 6, 2024

Conversation

yihui
Copy link
Contributor

@yihui yihui commented Sep 5, 2024

No description provided.

@yihui yihui marked this pull request as ready for review September 5, 2024 21:58
Copy link
Collaborator

@LittleBeannie LittleBeannie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comprehensive check and clean-up!

#' @importFrom mvtnorm GenzBretz
#' @importFrom stats pnorm qnorm setNames stepfun uniroot
#' @importFrom tibble tibble
#' @importFrom utils tail
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I prefer the redundant imports at the top of each function. It gives you a better sense of how widely used a given dependency is, and how reliant any given function is on a particular dependency. I'm fine with going with whatever style the group decides on, but I wanted to note that I view the redundancy as a feature.

R/gsDesign2-package.R Show resolved Hide resolved
data-raw/simu_test_gs_design_combo.R Outdated Show resolved Hide resolved
@nanxstats
Copy link
Collaborator

Apparently this is more of a personal preference and style guide thing so I'm afraid I can't be very helpful here.

The R Packages book dependencies chapter currently pushes a style that qualifies external namespaces with ::, without adding to NAMESPACE by default, unless it's used "a lot" or an operator. However, I don't think it's strictly enforced in their packages.

@yihui
Copy link
Contributor Author

yihui commented Sep 6, 2024

I don't really have a strong opinion on this. My original motivation was that I saw sometimes we use dplyr::, and sometimes we don't, so I wanted to clean up the unnecessary dplyr:: qualifiers. Then I moved on to remove other package qualifiers.

@jdblischak
Copy link
Collaborator

The R Packages book dependencies chapter

Later in that section, they also support defining @importFrom either separately for each function or in a central location. Thus I'm fine with migrating to the central location style.

@LittleBeannie LittleBeannie merged commit 60d6e9a into Merck:main Sep 6, 2024
7 checks passed
@yihui yihui deleted the import-once branch September 6, 2024 20:13
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