Skip to content
This repository has been archived by the owner on Sep 4, 2019. It is now read-only.

Get running on 0.4. #25

Merged
merged 6 commits into from
Jan 4, 2016
Merged

Get running on 0.4. #25

merged 6 commits into from
Jan 4, 2016

Conversation

sglyon
Copy link
Contributor

@sglyon sglyon commented Sep 17, 2015

Also some style adjustments to maintain consistency throughout library

Also some style adjustments to maintain consistency throughout library
@samuelcolvin
Copy link
Owner

Generally looks great. I would say:

  • tests are failing, looks (at least partly) like a vanilla julia error
  • Sorry my editor was set up wrong those spaces at the end of lines were/are very ugly.
  • do we still want 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?

@bryevdv
Copy link

bryevdv commented Sep 17, 2015

That's perfectly fine with me.

@sglyon
Copy link
Contributor Author

sglyon commented Sep 18, 2015

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.

@sglyon
Copy link
Contributor Author

sglyon commented Sep 18, 2015

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?

@samuelcolvin
Copy link
Owner

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
#25 (comment).

@sglyon
Copy link
Contributor Author

sglyon commented Sep 18, 2015

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

@samuelcolvin
Copy link
Owner

Ok, @spencerlyon2 I've invited you to the organisation, I'll review the PR over the weekend.

@sglyon
Copy link
Contributor Author

sglyon commented Sep 18, 2015

Great, thanks.

OK no rush. It is mostly cosmetic, with a couple things that are required to run on 0.4

@@ -2,4 +2,3 @@ julia 0.3
Mustache
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also change to julia 0.4- if you're dropping 0.3 support. When you tag, best to bump the minor version of the package just in case any 0.3 bugfixes are ever necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, will add.

Thanks (how do you keep up with changes to random packages before changes even hit metadata -- you are a machine!)

Copy link
Contributor

Choose a reason for hiding this comment

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

Had dinner with some continuum people last night so was reminded of this and thought I'd see what was new

@samuelcolvin
Copy link
Owner

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

@sglyon
Copy link
Contributor Author

sglyon commented Oct 19, 2015

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.

@sglyon
Copy link
Contributor Author

sglyon commented Oct 19, 2015

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

@andreasnoack
Copy link
Contributor

Bump

@samuelcolvin
Copy link
Owner

sorry for being useless. @spencerlyon2 shall we merge this? it's fine with me.

@sglyon
Copy link
Contributor Author

sglyon commented Jan 4, 2016

I think it is good to merge

sglyon added a commit that referenced this pull request Jan 4, 2016
@sglyon sglyon merged commit 22eeac0 into samuelcolvin:master Jan 4, 2016
@sglyon
Copy link
Contributor Author

sglyon commented Jan 4, 2016

@andreasnoack is #31 still necessary?

@andreasnoack
Copy link
Contributor

Great. I don't think #31 is necessary now.

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

Successfully merging this pull request may close these issues.

5 participants