-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
Fix index set parsing in JuMP macros on Julia 1.0. #1428
Conversation
This is a change introduced between Julia v0.7 and v1.0.
src/parseexpr.jl
Outdated
@@ -4,7 +4,11 @@ | |||
# file, You can obtain one at http://mozilla.org/MPL/2.0/. | |||
|
|||
function tryParseIdxSet(arg::Expr) | |||
if arg.head === :(=) | |||
if arg.head === :kw # Julia 1.0 onwards. |
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's better to use explicit VERSION checks so that they can be removed semi-automatically in the future.
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.
Should the version check be added for the old behaviour of the new behaviour?
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.
I don't think we have a convention, just as long as it's clear which code blocks can be removed when.
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.
LGTM pending CI
Codecov Report
@@ Coverage Diff @@
## master #1428 +/- ##
==========================================
- Coverage 88.93% 88.88% -0.05%
==========================================
Files 25 25
Lines 3677 3681 +4
==========================================
+ Hits 3270 3272 +2
- Misses 407 409 +2
Continue to review full report at Codecov.
|
For reference it was here: |
This is a change introduced between Julia v0.7 and v1.0.