Fix hygiene in objective macro #1517
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1517 +/- ##
==========================================
+ Coverage 89.44% 89.47% +0.02%
==========================================
Files 27 27
Lines 3639 3733 +94
==========================================
+ Hits 3255 3340 +85
- Misses 384 393 +9
Continue to review full report at Codecov.
|
|
Needs a test. Can we make sure all macros work instead of fixing them one by one? Does this affect 0.18.3? |
Yes, I am working on a better fix that defines two new functions that should be called at the end of every macros. The fix in getloopedcode wasn't sufficient
Yes, we will need to backport this fix |
src/macros.jl
Outdated
| """ | ||
| macro_return(model, code, variable) | ||
|
|
||
| Return a code that runs `code` in a local scope which returns the value of `variable`. |
There was a problem hiding this comment.
Nit: Return a block of code that 1. runs the code block code in a local scope and 2. returns the value of a local variable named variable. variable must reference a variable defined by code.
src/macros.jl
Outdated
|
|
||
| Return runs `code` in a local scope which returns the value of `variable` | ||
| and then assign `final_variable` to `escvarname`. | ||
| If `registerfun` is given, it registers the value returned by the local scope |
There was a problem hiding this comment.
registerfun is a function passed in. How do we know that it "registers the value returned by the local scope"?
The docstring should either provide the expected signature or call a concrete function.
src/macros.jl
Outdated
| $(if registerfun !== nothing | ||
| :($registerfun($model, $quotvarname, $variable)) | ||
| end) | ||
| # escvarname should be set in the scope calling the macr |
src/macros.jl
Outdated
| quotvarname, escvarname, final_variable=variable) | ||
|
|
||
| Return runs `code` in a local scope which returns the value of `variable` | ||
| and then assign `final_variable` to `escvarname`. |
There was a problem hiding this comment.
Could we take the varname and quotvarname as a single symbol and escape/quote it as needed?
There was a problem hiding this comment.
Indeed, I was able to get rid of quotvarname and escvarname. It makes things more readable to isolate this technical details in macro_assign_and_return :)
src/macros.jl
Outdated
| """ | ||
| macro_assign_and_return(code, variable, name; | ||
| final_variable=variable, | ||
| registerfun::Union{Nothing, Function}=nothing, |
test/Untitled.ipynb
Outdated
| @@ -0,0 +1,222 @@ | |||
| { | |||
|
Hi there, I'm the one who reported the issue at the discourse page. I can see that this PR has been merged into master, but I'm getting the exact same problems as reported on discourse when I use the master branch. I didn't have those problems in the original PR, however. FYI. |
Fixes the issue reported in https://discourse.julialang.org/t/undefvarerror-in-jump-constraint/15701/3