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

Refactor/update dots #1068

Merged
merged 8 commits into from
Mar 30, 2023
Merged

Refactor/update dots #1068

merged 8 commits into from
Mar 30, 2023

Conversation

kylebaron
Copy link
Collaborator

@kylebaron kylebaron commented Mar 21, 2023

Summary

This pull request refactors bad behavior that was in place when passing arguments to mrgsolve::update(). There was some bad reasoning behind the change introduced 4 years ago or so. It needs to be changed.

By default, there should be at least a warning issued when an argument is passed to mrgsolve::update() that the function doesn't recognize. Eventually, I think this should be an error, but historically it has been a warning so we'll keep it that way for now.

There was a (bad) option controlling this; the option was bad and the implementation was bad. So this PR deprecates the option. This is handled in the documentation where we describe package-wide options and there is a warning issued if the user passes a bad argument to mrgsolve::update() with the option set. I highly doubt anyone is using this option so I don't anticipate there will be downstream effects. But we'll warn if the option is used when the situation is right.

There is also an argument to mrgsolve::update() that controls if the warning should be issued when a bad argument is passed. This is really old, from when everything was organized quite different and we didn't have a very well defined model specification file. I think we might eventually drop that as well ... there is no need to make this behavior optional. So I wouldn't be against also deprecating the argument ... probably would be fine to just rip the band-aid off. But haven't done that (yet) in this PR.

@kylebaron kylebaron requested a review from kyleam March 29, 2023 02:21
Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

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

By default, there should be at least a warning issued when an argument is passed to mrgsolve::update() that the function doesn't recognize. Eventually, I think this should be an error, but historically it has been a warning so we'll keep it that way for now.

Makes sense, and the code changes look good to me.

One comment about something that's not new to this PR but related: shouldn't updates description for strict say "warning" rather than "error"?

mrgsolve/R/update.R

Lines 43 to 44 in 35ac94e

#' @param strict if \code{TRUE}, then an error will be generated if there is
#' attempt to update a non-existent item

It looks like it did error when that description was introduced in 33eab8f (2019-09-29) and then the description went stale in the next commit when 88c76c9 (warn when bad update is attempted from simulation function, 2019-09-29) demoted the error to a warning.

@kylebaron
Copy link
Collaborator Author

Thanks @kyleam ; i'll update that and put in a TODO to finally clean this up.

@kylebaron kylebaron merged commit 0bfa6d3 into main Mar 30, 2023
@kylebaron kylebaron deleted the refactor/update-dots branch March 30, 2023 00:24
@kylebaron kylebaron self-assigned this Jul 15, 2023
@kylebaron kylebaron added this to Done in Development Jul 15, 2023
@kylebaron kylebaron added this to the v1.1.0 milestone Jul 15, 2023
@kylebaron kylebaron mentioned this pull request Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants