-
-
Notifications
You must be signed in to change notification settings - Fork 401
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 nice fallback for build_variable #2451
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2451 +/- ##
==========================================
- Coverage 92.25% 92.16% -0.09%
==========================================
Files 43 43
Lines 4634 4610 -24
==========================================
- Hits 4275 4249 -26
- Misses 359 361 +2
Continue to review full report at Codecov.
|
Just commented here #2421 (comment) why I think we should not have multiple positional args. |
"you need to implement `build_variable`." | ||
) | ||
end | ||
|
||
function build_variable(_error::Function, variable::ScalarVariable, |
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.
Note that this build_variable
is not documented, and as in the comment in #2421 (comment)
It could be build_variabe_in_set
. It would be probably breaking though.
It might be even good not to document to keep it less breaking.
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.
it seems you hit a similar problem in: #2450
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.
Yeah, it's staying for now. It should be obvious that it should change if we refactor the macros.
Closes Part I of #1884 (The related issue is still unresolved.)