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

match.arg is not behaving as I thought it did #289

Merged
merged 36 commits into from
Jul 30, 2024
Merged

Conversation

CeresBarros
Copy link
Member

@CeresBarros CeresBarros commented Jul 10, 2024

match.arg won't error if arg contains entries that are not found in choices -- it simply excludes them. It also does a partial which may lead to selecting the wrong options (especially for variable names if the user partially writes them right.

This is not the type of behaviour we want most times so I replaced many of the match.arg with setdiff-type of checks.

My local checks are failing to build one of the vignettes, but I doubt that this is related to my changes - keeping this as a draft to diagnose.

Also, please note that I've added 00 to the development version numbering (it should be 9000, 9001, 9002, ..., not 90, 91, 92, ...)

@CeresBarros CeresBarros requested a review from kdaust July 10, 2024 23:06
@CeresBarros CeresBarros marked this pull request as draft July 10, 2024 23:06
@CeresBarros
Copy link
Member Author

climr_workflow_int.Rmd is failing to knit with RStudio build tools, but not if I knit it manually -- so possibly okay?
Marking PR as ready to review

@CeresBarros CeresBarros marked this pull request as ready for review July 10, 2024 23:10
@CeresBarros
Copy link
Member Author

uhmm i see the same vignette build failing with a different error now on GHA:

Error: Error in proc$get_built_file() : Build process failed

@CeresBarros
Copy link
Member Author

CeresBarros commented Jul 19, 2024

other fixes have been added to this PR:

  • system.file doesn't find the file if devtools::load_all was used to load climr
  • removal of nested function definition
  • added missing imports and global variable definitions
  • streamlining of code in plot_timeSeries_input and related internal functions

@CeresBarros
Copy link
Member Author

all tests seem to be passing now, except code coverage calculation -- not sure what's going on, but not serious.

@cmahony before merging, could you please check that the plot_timeSeries_input function is behaving as intended? I did some important code changes (streamlining, readability, moved internal function definitions to "outside") and things look good to me (the code in te @examples) but perhaps having your eyes on it would be better. Thanks :)

@CeresBarros
Copy link
Member Author

approval on plot_timeSeries function (not plot_timeSeries_input, my mistake on an earlier comment) by @cmahony -- all code checks are passing, so I'm merging.

@CeresBarros CeresBarros merged commit cee1f17 into bcgov:devl Jul 30, 2024
4 of 5 checks passed
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.

1 participant