-
Notifications
You must be signed in to change notification settings - Fork 50
Add missing LineNumberNode to @swappable call (fixes #257) #259
Conversation
Thanks. Indeed this fixes loading the package, but I wonder whether the test failure may be related: the path between
|
Don't think the test error is related. Actually, it seems to me the test should Fail (but not error), as 1 == NA should be NA. But this discussion is for the other issue. |
src/operators.jl
Outdated
@@ -138,7 +138,11 @@ macro dataarray_binary_scalar(vectorfunc, scalarfunc, outtype, swappable) | |||
if swappable | |||
# For /, Array/Number is valid but not Number/Array | |||
# All other operators should be swappable | |||
map!(x->Expr(:macrocall, Symbol("@swappable"), LineNumberNode(@__LINE__), x, scalarfunc), fns, fns) | |||
if VERSION < v"0.7.0-" |
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 would be great if you could identify the exact commit which broke this (use contrib/commit-name.sh
).
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.
My guess is the exact commit is: 0.7.0-DEV.328 . Specifically, JuliaLang/julia@fcdf437
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.
You should use the merge commit JuliaLang/julia@9e3318c. Its master parent commit, JuliaLang/julia@8b2cac4, is 0.7.0-DEV.353 but does not have this change.
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.
Damn, everybody gets this wrong at least once. Do you think the script could do this automatically?
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.
we could maybe have it do some git operations to try to figure out whether the commit it runs on is a merge (or squash merge) commit or something from a PR branch and give a warning, but I'm not sure what to check for
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.
If github would fix "rebase and merge" to include a reference to the PR number, we could switch to that for multi-commit PR's and disable the normal merge commit option. Single-commit PR's should almost always be squash merged since there's no reason not to, so it's only multi-commit PR's where there's a risk of VERSION getting things wrong.
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.
did this get fixed?
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.
No, but see #264.
You are right
|
I guess we can still merge this (after updating the version check) and continue debugging, as it's always easier to debug a package that can be loaded. |
Thanks for figuring this out! |
The failure actually comes from a change in Base, see JuliaLang/julia#22019 and JuliaLang/julia#22130. |
There has been some change to Julia in the macrocall AST, adding the LineNumberNode.
Unless there is another change to Julia, some packages using
:macrocall
expression generation will break.