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

Args validation checks #116

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Args validation checks #116

merged 4 commits into from
Jan 31, 2024

Conversation

siddhesh195
Copy link
Contributor

The change adds validation check when duplicate names args are passed to animint
Closes #83

Validation check for duplicate arguments
Add test for error message thrown by animint()
R/z_print.R Outdated
duplicate_args <- names(L)[duplicated(names(L))]

if (length(duplicate_args) > 0) {
stop(paste("Error: Duplicate arguments are passed to animint. Duplicate arguments found :",
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like a good start thanks!
please remove Error: (that will show up anyway when you use stop) and please remove space before colon
"found :" -> "found:"
also next time can you please commit and push the test code so that we can see that it fails on github actions, before you push the code which makes the test pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the error message. Next time, I will push the test code first before pushing the code

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks good thanks!

@tdhock tdhock merged commit 7e6ec29 into master Jan 31, 2024
5 checks passed
@tdhock
Copy link
Collaborator

tdhock commented Jan 31, 2024

I should have asked you to do that here before merging, but can you also in another PR please

  • add your name as contributor in DESCRIPTION
  • increase version
  • add a NEWS item
  • make a test and fix for empty animint()

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.

un-informative error when out.dir specified twice
2 participants