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

Updated for 1.0 #54

Merged
merged 8 commits into from
Aug 16, 2018
Merged

Updated for 1.0 #54

merged 8 commits into from
Aug 16, 2018

Conversation

mlhetland
Copy link
Contributor

Updated to use new iteration protocol, if available. Importing done creates problems otherwise.

@coveralls
Copy link

coveralls commented Aug 9, 2018

Coverage Status

Coverage decreased (-0.2%) to 78.687% when pulling fbfeca4 on mlhetland:1.0-fixes into b2f76ee on dcjones:master.

@mpastell
Copy link

Any change for merging this and tagging a new release soon? It would be great to get Weave.jl running on 1.0 and it depends on this package.

@mlhetland
Copy link
Contributor Author

It'd be great for me to have this work by the end of the week; otherwise, I'll be using 0.7 in my course this semester :-}

@sglyon
Copy link
Collaborator

sglyon commented Aug 13, 2018

Thank you for submitting.

Will you please enable tests on julia 0.7 and 1.0? To do this we need to change these lines to also include - 0.7 and - 1.0

Once we see that this fix changes tests we will be good to merge.

@mlhetland
Copy link
Contributor Author

It seems it's the test infrastructure itself that fails, as it uses Pkg in a pre-1.0 way?

@mlhetland
Copy link
Contributor Author

That is, rather than julia -e 'Pkg.clone(pwd()), it should be julia -e 'using Pkg; Pkg.clone(pwd()), though only for Julia 1.0, so unless you can have different commands for different versions, I guess you need an if VERSION thing in there as well.

@mlhetland
Copy link
Contributor Author

The following should work:

julia -e 'if VERSION >= v"0.7" using Pkg end; Pkg.clone(pwd())

@mpastell
Copy link

@mlhetland yes, that's what some other packages seem to use.

@mlhetland
Copy link
Contributor Author

Maybe something @sglyon wants to address? (Or should I change it? Not sure where the Travis infrastructure is, ATM, but I’m sure it’s possible to find it :-))

@sglyon
Copy link
Collaborator

sglyon commented Aug 13, 2018

I think we should make that change -- it is what I would do myself so if you are willing to do it on this branch that'd be great!

@mlhetland
Copy link
Contributor Author

Didn't notice that it was all just further down in the Travis-file xD Fixed.

@mlhetland
Copy link
Contributor Author

Hm. I thought I tested it with 1.0, but it seems maybe I only tested with 0.7 for deprecation warnings. Still importing start and next, which don't exist in 1.0. Sorry about that! I'll do some bug-fixing an test properly in 1.0 :-)

@mlhetland
Copy link
Contributor Author

mlhetland commented Aug 13, 2018

Well, one problem is that we'll need Codecs#master. I guess they haven't tagged a release with the fixes there yet.

@mpastell
Copy link

Codecs has been tagged, but not merged JuliaLang/METADATA.jl#16489 .

@mlhetland
Copy link
Contributor Author

Oh, OK.

@mlhetland
Copy link
Contributor Author

Anyway: I've tested with Julia 0.6, 0.7 and 1.0, and the tests run fine locally for me. But then, of course, I use Compat for 0.6 and Compat#master for 0.7 and 1.0.

@mlhetland
Copy link
Contributor Author

So I guess as soon as we get the right version of Codecs, the tests here should pass.

@sglyon
Copy link
Collaborator

sglyon commented Aug 14, 2018

Sounds great, thank you for all the work here. Please notify us on this thread when I should review and prepare to merge

@sglyon sglyon mentioned this pull request Aug 15, 2018
@TransGirlCodes
Copy link
Contributor

What is the status of this PR please? We need a 1.0 compatible version of YAML for BioJulia packages ASAP, but don't want to tread on toes of people already working on PRs :)

@TransGirlCodes
Copy link
Contributor

Codecs.jl v0.5.0 has been release which works for julia v0.7 / v1.0, if this PR uses that version it should pass?

@mlhetland
Copy link
Contributor Author

The holdup has been that Codecs 0.5 hasn't been merged into METADATA. If you can specify specific tags or branches in your requirements, you could always just use this PR and 0.5 of Codecs directly. I could add a tag, if that's useful (i.e., rather than checking out the branch). But using Codecs#v0.5.0 and https://github.com/mlhetland/YAML.jl#1.0-fixes currently works.

As soon as Codecs 0.5 is in METADATA, the checks here should pass, I guess, yes.

@sglyon
Copy link
Collaborator

sglyon commented Aug 15, 2018

Ok great, thank you for the update

@mlhetland
Copy link
Contributor Author

I’m on my phone right now, so it’s a bit awkward to check, but I believe Codecs 0.5 should now be available. Can we manually re-run the checks? (It seems they’re stale at the moment?)

@TransGirlCodes
Copy link
Contributor

@mlhetland Codec 0.5 is indeed available now. If you have access to the travis you re-run the CI checks.

@mlhetland
Copy link
Contributor Author

Hm. If I do have access, I at least can’t figure out how. (Wouldn’t I need write access to this repo? Am I missing something?)

@mlhetland
Copy link
Contributor Author

Just thought I'd push a change to trigger a new check – so I added a Project.toml file. :-)

@mlhetland
Copy link
Contributor Author

Being too clever here, I guess – breaking stuff. (Tricky when I can't test the CI stuff locally…) The deprecation warning for clone said to use add, but that doesn't work with a path, it seems. (Maybe I'm just using it wrong.) And there was an issue with developing a package with the same UUID as the project (which I guess makes sense – though here it is the same project), so I just removed the Project.toml file, again. I think it's correct – it works fine with, say, instantiate; it's just that the Travis setup had issues with it. I guess it can be re-added later.

@mpastell
Copy link

See e.g Weave for travis config that works with project.toml https://github.com/mpastell/Weave.jl/blob/master/.travis.yml

@mlhetland
Copy link
Contributor Author

Ah – yeah, I looked at Weave (since I assumed you were up to date, given the discussion earlier :-). I just didn't want to hack around too much, and wasn't sure which parts worked with Julia 0.6 (given that some parts of it don't). If you have a suggested edit for the YAML Travis file, I'd be happy to implement it :-)

@mlhetland
Copy link
Contributor Author

I'm having a look at it now.

@mlhetland
Copy link
Contributor Author

All done, and ready to merge! (cc @sglyon)

@mlhetland
Copy link
Contributor Author

Now that YAML.jl is in BioJulia, maybe @benjward can merge this?

@sglyon sglyon merged commit 8355307 into JuliaData:master Aug 16, 2018
@sglyon
Copy link
Collaborator

sglyon commented Aug 16, 2018

Merged thank you!

@TransGirlCodes
Copy link
Contributor

Thanks for merging @sglyon

@mlhetland
Copy link
Contributor Author

Now getting a release would be great, so this becomes available without using YAML#master… :-> (BTW, my Project.toml already says version = 0.2.2.)

@mlhetland mlhetland deleted the 1.0-fixes branch August 16, 2018 12:04
@TransGirlCodes
Copy link
Contributor

TransGirlCodes commented Aug 16, 2018 via email

@TransGirlCodes
Copy link
Contributor

Actually the version number is going to be v0.3.0, since julia 0.5 support is dropped, and previous v0.2.x versions supported julia 0.5, METADATA CI is making me increment the minor version number (as it should to be fair!).

@mlhetland
Copy link
Contributor Author

Ah, sure. You should edit Project.toml so it has version = "3.0.0", then :-)

mlhetland added a commit to mlhetland/YAML.jl that referenced this pull request Aug 27, 2020
* Updated for 1.0

* Add tests for 0.7 and 1.0

* Add Pkg imports in 0.7+

* Remove start, next from 1.0

* Add Project.toml

* Use Pkg.add in .travis.yml

* Temporarily remove Project.toml

* Reintroduce Project.toml
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.

5 participants