Closed sglyon closed 8 years ago
Generally looks great. I would say:
compat
? Surely with the imminent release of 0.4 we should just go straight to 0.4 julia? I doubt enough people are using it with v0.3 who won't be able to move straight away to v0.4.Also @bryevdv: @spencerlyon2 has offer to push forward with Bokeh.jl while I'm being useless, it looks like I'm allowed to add him to the bokeh "bindings" team, is that ok with you?
That's perfectly fine with me.
Nice! I was hoping you'd be up for targeting just 0.4. I'll clean things up then and add to this PR.
Tests passed for me locally, I'll investigate why they are failing.
So from the travis report, the test error comes on 0.3 only because I forgot to put @compat
right here.
If we are ready to drop 0.3 support. I think we could just remove all compat stuff within this PR and turn travis off on 0.3.
What do you think?
Go for it. On 18 Sep 2015 3:24 am, "Spencer Lyon" notifications@github.com wrote:
So from the travis report, the test error comes on 0.3 only because I forgot to put @compat right here https://github.com/bokeh/Bokeh.jl/pull/25/files#diff-0140e875b1bf83d60a0f098629b13883R102.
If we are ready to drop 0.3 support. I think we could just remove all compat stuff within this PR and turn travis off on 0.3.
What do you think?
— Reply to this email directly or view it on GitHub https://github.com/bokeh/Bokeh.jl/pull/25#issuecomment-141310768.
OK removed all compat stuff. Getting ready to go on the 0.4 rc I also replaced String
with AbstractString
and Nothing
with Void
.
I updated the travis file to only run on 0.4 and nightly (0.5). We can disable the nightly one if we want.
Also removed Compat from REQUIRE
Ok, @spencerlyon2 I've invited you to the organisation, I'll review the PR over the weekend.
Great, thanks.
OK no rush. It is mostly cosmetic, with a couple things that are required to run on 0.4
a few small things to check, but lets try to get this fixed and merge fairly soon, then do a new release to support v0.4
Took care of comments on diffs.
Any more changes we need to make before merging?
Besides fixing the bug I just introduced ;)
I'm on that, as well as working through deprecation warnings. Will post again when I'm done.
sorry this just got noisier -- I changed all Union(...)
to Union{...}
.
But, locally tests pass and I have no deprecation warnings. So, assuming tests pass on travis also I think this is good to go
Bump
sorry for being useless. @spencerlyon2 shall we merge this? it's fine with me.
I think it is good to merge
@andreasnoack is #31 still necessary?
Great. I don't think #31 is necessary now.
Also some style adjustments to maintain consistency throughout library