emmt commented 5 years ago

This PR is about an update of FITSIO.jl so that it can be used with Julia 0.6, 0.7 and 1.0.

I have checked that all tests pass successfully with Julia versions 0.6.4, 0.7.0 and 1.0.0 with the following code:

using Pkg # of course, not for Julia 0.6
Pkg.clone("https://github.com/emmt/FITSIO.jl.git")
Pkg.build("FITSIO")
Pkg.test("FITSIO")

There are many changes, which explain that the PR is split in several commits. Hopefully these changes do not affect the API at the user level (as a proof: appart from using Compat, the file test/runtests.jl is almost unchanged).

Due to the deprecation of Nullable{T}, I had to make a choice for the internal API of a few methods used to parse FITS header values. The Julia recommendation is to replace Nullable{T} by Union{Some{T},Nothing} to distinguish whether the return value is a true Nothing or is missing. This however leads to a somewhat overcomplex code, it seemed more simple to follow the behavior of tryparse(T,s) which (as of Julia version ≥ 0.7) yields either a value of type T or nothing. Thus the following behavior is currently implemented:

The methods try_parse_hdrval(T,s) behave as tryparse(T,s).
The method try_parse_hdrval(s) returns a value of type String, Bool, Int, Float64 or Nothing. The latter occurs if the value string s is empty or invalid.
The method parse_header_val(s) returns a value of type String, Bool, Int, Float64 or Nothing. The former occurs (a String) occurs if the value string s is a FITS string or if it is invalid. The latter occurs if the value string s is empty.

This works but this choice is not completely satisfying because the result of parse_header_val(s) is ambiguous. I would rather have method parse_header_val(s) return a value of type String, Bool, Int, Float64 or Nothing (if value is empty) and throw an error if value cannot be parsed and remove method try_parse_hdrval(s) (with a single argument). Throwing an error is necessary because there is no other means to realize that someting wrong occured. These methods are private (in the sense that they are not in the official documentation) so that this choice should not affect end users. More generally, we should have a discussion about how to report errors when reading a FITS keyword because there are 2 cases of failure: either the keyword is not found or its value cannot be parsed. Throwing distinct exceptions for these cases could solve the issue.

I foresee to make a further PR along these lines. Otherwise, I suggest to make a new official release (perhaps after a short period of testing) because many users (especially the new ones) will use Julia 1.0 now it has been released.

giordano commented 5 years ago

Thank you! I hadn't gone through the changes, however I'd like to point out that there is already another open PR which addresses the same issue: #101. We should probably merge them somehow

giordano commented 5 years ago

In addition, if there are no functional changes to the package, I'd drop support for julia 0.6 completely, as it was done in #101.

emmt commented 5 years ago

I think that dropping support for 0.6 is not yet a good idea. There are so many incompatibilities between 0.6 and 0.7 or 1.0 that it may take some time before users convert all their software to use the latest Julia versions.

gcalderone commented 5 years ago

I agree, for instance WCS has not yet been ported to 0.7/1.0.

I'm happy to see that someone is actively working to port FITSIO on 0.7, thank you very much for your efforts! Do you have any idea how long will it take to merge this PR and/or #101 ?

101 has been there for more than two months now....

giordano commented 5 years ago

I think that dropping support for 0.6 is not yet a good idea. There are so many incompatibilities between 0.6 and 0.7 or 1.0 that it may take some time before users convert all their software to use the latest Julia versions.

Ok, but I'm not sure we're going to add new features in the short run. Sropping support for Julia 0.6 doesn't mean users won't be able to install FITSIO.jl at all in Julia 0.6, previous versions will still work.

giordano commented 5 years ago

Do you have any idea how long will it take to merge this PR and/or #101 ?

101 has been there for more than two months now....

101 is marked as WIP because @mweastwood has been busy with his thesis, but should hopefully be over now. Michael, can you have a look at this?

emmt commented 5 years ago

I can have a look at #101 and see if it has features not in #103 and then try to merge them to keep the most of the two. However I do not know how to merge the two with git so as to preserve "who did what".

giordano commented 5 years ago

That would be great! I had a quick look to this PR and looks quite complete.

If you want to give attribution to Michael, you can use the co-authored cookie in the commit message: https://blog.github.com/2018-01-29-commit-together-with-co-authors/

emmt commented 5 years ago

OK to use the suggested cookie (thanks!) if Michael agrees.

But I just add a quick look at the git doc. and at #101 and there are not so many differences with my #103. So another possibility is to fork FITSIO.jl then apply #101 (hence with reference to Michael's work) and then modify it to add my own (remaining) changes. This could be quick.

giordano commented 5 years ago

For the appveyor script, please use https://github.com/JuliaCI/Appveyor.jl it's much simpler than the current one, you just have to set the tested version of julia (and allowed failures, if needed), that's all

emmt commented 5 years ago

Thanks for the hint.

For now, I have merged #101 and #103 in a new PR (#104). To do that, I simply fetched #101 in a new contrib branch of my fork of FITSIO.jl, made my additional changes and issued a new PR. It was quite easy. I recall the commands below in case it may help others.

# A few settings to make things easier ("origin" is my fork on GitHub,
# "upstream" is the official repostory):
git config remote.pushdefault origin
git config push.default current
git remote add upstream 'https://github.com/JuliaAstro/FITSIO.jl.git'

# Retrieve pull request 101 of the  official repository in a new local
# branch named "contrib":
git fetch upstream pull/101/head:contrib

# Checkout this branch, make my changes, commit them and push them
# to my GitHub fork before using GitHub GUI to make the PR:
git checkout contrib
# edit some files
git commit -a
git push

I am closing this PR as #104 supersedes it.

JuliaAstro / FITSIO.jl

Update FITSIO.jl to work with Julia 0.6, 0.7 and 1.0 #103

101 has been there for more than two months now....

101 has been there for more than two months now....

101 is marked as WIP because @mweastwood has been busy with his thesis, but should hopefully be over now. Michael, can you have a look at this?