-
Notifications
You must be signed in to change notification settings - Fork 81
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 #193, issue with makeform macro in v0.5 #194
Conversation
This looks OK to me. I'll merge this imminently unless there are any objections. |
Actually, thinking about this a little more, shouldn't this do different things depending on Julia version? Rather than layering the new behaviour on top of the old? This might cause problems later? Thanks @ScottPJones for tacking this... |
@aviks 😊 Thanks! I don't think this could cause a problem, because I believe the AST for parsing |
Thanks for looking at this @aviks. I am struggling with bandwidth issues (my own, not my ISP) at the moment, and I am really grateful for the help. |
(but do let me know if you need me for some reason) |
Yes, indeed, but my worry is that the old form might be re-purposed in the future for some other use. In this particular case, that may be unlikely; but it seems better hygiene to protect this. no? |
OK, I'll try to narrow down just which of the many v0.5 commits changed the AST output, but that's going to be a good deal more work. |
This indeed is because of JuliaLang/julia#15524, I just need to get the correct version number now to fix this as you requested @aviks, will be ready shortly. |
@aviks, all set now, using |
Thanks. I restarted the travis build of the last master commit, and that failed tests. This one passes. So looks good. |
The AST that it was inspecting had changed in v0.5 with recent changes, so that it didn't detect the
in
operators correctly.I also fixed another issue that caused a warning in v0.5 (readall has been deprecated and renamed readstring)