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

Attempt to fix Julia v0.6 support #54

Merged
merged 5 commits into from
May 5, 2017
Merged

Conversation

rdeits
Copy link
Contributor

@rdeits rdeits commented May 3, 2017

There were some changes to the way keyword arguments are parsed in Julia v0.6 that broke Logging.jl. I believe this should fix it, but I'd appreciate it if someone familiar with how the package works could try it as well (the unit tests are passing, but I don't know the expected behavior well).

Copy link
Owner

@kmsquire kmsquire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tacking this!

@@ -142,11 +143,20 @@ end

override_info(;args...) = (:override_info, true) in args

# Keyword arguments x=1 passed to macros are parsed as Expr(:(=), :x, 1) but
# must be passed as Expr(:(kw), :x, 1) in Julia v0.6.
@static if VERSION < v"0.6-"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@static probably isn't necessary here (although I doubt it hurts anything).

@test ex |> macroexpand == :nothing
end
end
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@kmsquire kmsquire merged commit b4809ec into kmsquire:master May 5, 2017
@kmsquire
Copy link
Owner

kmsquire commented May 5, 2017

I think this is a step in the right direction, but I probably can't release a version yet. Julia has moved more strongly to the convention that packages should not overwrite Base functions, so the fact that we (intentionally) overwrite Base.info and Base.warn probably isn't going to fly.

I'll try to commit some time to address this issue over the weekend.

(You also might consider using Memento.jl, which seems not to have this issue and looks to be quite well written.)

Cheers!

@rdeits
Copy link
Contributor Author

rdeits commented May 5, 2017

Ok, thanks! Memento.jl does look quite nice, and I've been looking at the new logging features in Base too. But my goal here was just to un-break Mocha.jl :)

kmsquire pushed a commit that referenced this pull request Jun 2, 2017
* fix keyword handling in julia v0.6
* use Compat: @static
* macro hygiene fixes
* ensure that macros work inside modules too
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.

2 participants