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

Add {checkmate} dependency for function argument checks #175

Closed
averissimo opened this issue Aug 3, 2023 · 1 comment · Fixed by #190
Closed

Add {checkmate} dependency for function argument checks #175

averissimo opened this issue Aug 3, 2023 · 1 comment · Fixed by #190
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@averissimo
Copy link
Collaborator

averissimo commented Aug 3, 2023

xportr would benefit from some argument checks, first to the exported functions and maybe on a second phase to internal functions

Why?

To ensure a proper use of the package and help users/developers avoid problems with usage and development.

Effort?

I wouldn't expect this would be a very complex change and I'm volunteering for it, if the team thinks it's the right path

Examples

Example (function arguments)

.internal_verbose_choices <- c("none", "warn", "message", "stop")

# ...
xportr_label <- function(.df,
                         metadata = NULL,
                         domain = NULL,
                         verbose = getOption("xportr.label_verbose", "none"),
                         metacore = deprecated()) {
  checkmate::assert_data_frame(.df) # probably this would have to be deferred to after `enexpr` call
  checkmate::assert(
    combine = "or",
    checkmate::check_r6(metadata, "metacore", null.ok = TRUE),
    checkmate::check_data_frame(metadata, null.ok = TRUE)
  )
  checkmate::assert_string(domain, null.ok = TRUE)
  checkmate::assert_choice(verbose, choices = .internal_verbose_choices)

Example (small inner changes):

if (!is.null(domain) && !is.character(domain)) {

Currently, the if clause would accept a vector of characters as domain (c("some", "vector", "of", "domains")) and it would have some unwanted effect.

The change in code would be:

if (checkmate::test_string(domain)) {

Some other changes that would be possible are the checks for integerish numbers (for lengths validation) instead of just checking if it's numeric.

@averissimo averissimo added enhancement New feature or request question Further information is requested labels Aug 3, 2023
@averissimo averissimo changed the title Add {checkmate} for function argument checks Add {checkmate} dependency for function argument checks Aug 3, 2023
@bms63
Copy link
Collaborator

bms63 commented Nov 16, 2023

@averissimo could we add this to our wiki for programming strategy? it is pretty light, but nice to document somewhere that new features should use this.

also could we avoid the :: and add checkmates functions here https://github.com/atorus-research/xportr/blob/main/R/xportr-package.R. this is just the convention we adopted in xprotr

bms63 added a commit that referenced this issue Jan 23, 2024
Closes #175 Adds assertions to exported functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants