Skip to content
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

Merged
merged 3 commits into from
Mar 28, 2016

Conversation

ScottPJones
Copy link
Contributor

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)

@ScottPJones
Copy link
Contributor Author

@timholy I noticed it looks like you have merge privileges on @dcjones' repos. Could you please take a look at this PR, and also the PRs #195 & #196? I'd been trying to get things working again in v0.5, I think a lot of people are using Gadfly and this is part of getting it working again. Thanks!

@aviks
Copy link
Collaborator

aviks commented Mar 27, 2016

This looks OK to me. I'll merge this imminently unless there are any objections.

@aviks
Copy link
Collaborator

aviks commented Mar 27, 2016

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...

@ScottPJones
Copy link
Contributor Author

@aviks 😊 Thanks! I don't think this could cause a problem, because I believe the AST for parsing (x in xs) could only be in either the old form or the new form.

@timholy
Copy link
Collaborator

timholy commented Mar 28, 2016

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.

@timholy
Copy link
Collaborator

timholy commented Mar 28, 2016

(but do let me know if you need me for some reason)

@aviks
Copy link
Collaborator

aviks commented Mar 28, 2016

could only be in either the old form or the new form

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?

@ScottPJones
Copy link
Contributor Author

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.
Would it be at all possible to merge this in, just to get things working again for people on v0.5, with a definite promise from me to submit a PR as soon as I've figured out which commit & version number to check for?
The other two PRs I submitted are needed independent of Julia version, BTW.
Thanks again, both of you!

@ScottPJones
Copy link
Contributor Author

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.

@ScottPJones
Copy link
Contributor Author

@aviks, all set now, using VERSION, as you asked for. Thanks for reviewing!

@aviks aviks merged commit 2109bf8 into GiovineItalia:master Mar 28, 2016
@aviks
Copy link
Collaborator

aviks commented Mar 28, 2016

Thanks. I restarted the travis build of the last master commit, and that failed tests. This one passes. So looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants