-
Notifications
You must be signed in to change notification settings - Fork 92
code style in checking internals #778
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
Conversation
Thanks for making this happen, @qiushiyan! Feel free to mark this PR as ready for review when you feel good about it and I'll take a look. I had wondered if there were any conditions that this might happen when I put this together, and it looks like I might have missed one. The argument checking across model spec functions is indeed tricky. :) |
Now compute inform message at print time in |
Awesome, thanks for making this happen @qiushiyan. Will review and move this along next week. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly reviewing for myself to point out notable changes.
Lots of helpful syntax consistency changes here, though the diffs are a bit difficult to navigate because of them.
Otherwise, as is, relocates check_missing_spec
from new_model_spec
to print.model_spec
, shortens the missing extension package message, and makes needed adjustments!
From here, as Qiushi mentioned, need to figure out actions that indicate defining a model spec is "finished" so we know we can check the mode
vs engine
vs class
. fit
is one of those places.
Splitting this PR into two, one for code style edits and one to improve messaging for missing implementations. This one is now the former, so will go ahead and merge! |
This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue. |
When
mode == 'unknown'
,inform_missing_implementation
searches available models for all modes.Also had to use
new_model_spec(check_missing_spec = FALSE)
inboost_tree
(and perhaps other models that involve extension packages) andnew_model_spec(check_missing_spec = TRUE)
inset_engine.model_spec()
, such that inform doesn't use the default engineCurrent error messages
Created on 2022-08-01 by the reprex package (v2.0.1)
One remaining problem being if
set_mode
is called afterset_engine
then the user specified mode won't be reflected in the error message. Ifset_mode
usesnew_model_spec(check_missing_arg = TRUE)
as well then we get two sets of messages.