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 function input checks #45

Closed
wants to merge 2 commits into from
Closed

Add function input checks #45

wants to merge 2 commits into from

Conversation

matt-dray
Copy link
Collaborator

@matt-dray matt-dray commented Dec 14, 2023

Closes #28.

Given the discussion in #28, I'm creating this draft pull request for comments on the approach.

In short, checks are in the form check_*(..., .other_args). The dots permit any number of variables, which should reduce the number of individual calls to a given check and allow for comparison of multiple values (e.g. for ensuring their lengths match).

This PR will also introduce an import: {cli}. It makes constructing and communicating errors easier and is in use throughout the tidyverse suite.

If this PR is successful, the tests drafted by @chrismainey in #39 should be instigated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a case here where specifying a vector of length >1 into target_wait, and relying on default factor =4. Would this defeat check_lengths_match()? Might need a check on the use of default and extend to same length vector.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issue as average wait regarding the use of the default value into check_lengths_match

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issue as average wait regarding the use of the default value into check_lengths_match

Copy link
Collaborator

@chrismainey chrismainey left a comment

Choose a reason for hiding this comment

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

Great approach @matt-dray . I'm stealing those utils functions for other packages!
My only thought is the check_lengths_match() part can be tripped up if using vectors of length >1 for inputs, but also using the default value from things like factor or F. Should we add additional checks and extend the default to a vector of the same length?

@matt-dray
Copy link
Collaborator Author

Thanks for this @chrismainey. On reflection, do we really want to error if argument lengths don't match? This conflicts with R's recycling behaviour and may surprise users.

Of course, R warns if there's 'non-multiplicative' recycling:

average_wait(1:3, 1:2)
# Warning message:
# In target_wait/factor :
#  longer object length is not a multiple of shorter object length

Perhaps it would be better to intercept that warning and provide a more meaningful message?

@matt-dray
Copy link
Collaborator Author

Also, for interest: in commit 259dd4b I've replaced the gnarly 'dots extraction' method with something much simpler and used Nick's approach to prevent the 'error-handling eclipse' code smell.

Copy link
Collaborator

@ThomUK ThomUK left a comment

Choose a reason for hiding this comment

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

I think these input checks are a good addition, and we should merge and clean up any gotchas in a followup issue.
The linting has caused a list of merge conflicts to resolve, but if it's only the linting that has caused them they should be easy to resolve.

@matt-dray
Copy link
Collaborator Author

Thanks @ThomUK, but closing in favour of #50 (see reasoning in that PR).

@matt-dray matt-dray closed this Mar 11, 2024
@matt-dray matt-dray deleted the 28-check-function-input branch March 11, 2024 12: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.

Add function input checks
3 participants