Fix #101 to support Julia 0.6, 0.7 and 1.0 #104
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We still get one deprecation warning for `Base.Libdl`, but these come from `BinaryProvider`.
Appart from trivial changes to avoid deprecation warnings (for things allowed in 0.7 but forbidden in 1.0) and to maintain compatibility with Julia 0.6, the main changes/fixes are: * When closing a FITS file, set `f.ptr` to `C_NULL prior to checking whether there was an error. This is to avoid trying to close more than once in case an error occurs. * Fix FITS handle being used as an iterable (i.e., extend method `iterate` instead of methods `done`, `next` and `start`). * Method `try_parse_hdrval(s)` (with a single string argument) is not used anywhere and has been deleted and its code moved into method `parse_header_val`. Note that methods `try_parse_hdrval` can be assumed to be *private* as they are not documented in [FITSIO doc.](http://juliaastro.github.io/FITSIO.jl/latest/) and are only called in the file `header.jl`.
|
This is great! I hope I'll be able to have a look tonight. |
|
Thanks for picking up where I left off! This looks great. |
|
Ok, I think we've waited enough considered that #101 has been open for some months now, I'm going to merge this. Thanks again @emmt and @mweastwood! |
|
@emmt You've outlined some good future improvements: please put them in a new issue, so they won't be lost here 😉 |
|
Yes I will do that. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is both a review and a fix of #101 to extend FITSIO support to all Julia versions ≥ 0.6
The fixes are summarized in the commit comments. The proposed PR passes all the tests with Julia versions 0.6.4, 0.7.0 and 1.0.0.
For the review part, I fully agree with most of the changes done in #101. In fact, they are quite similar to my changes in #103, thus this new PR is a candidate for replacing #101 and #103. In #101 and #103. As discussed in #101 and #103, the most significant changes concern the
try_parse_hdrvalmethods:Methods
try_parse_hdrval(T,s)(with a type and a string arguments) used to return aNullable{T}result and now returns either a value of typeTornothingif unsuccessful. This follows the behavior of thetryparsemethods in latest Julia versions.Method
try_parse_hdrval(s)(with a single string argument) was only called byparse_header_val. It has been deleted (in this PR) and its code moved into methodparse_header_val.Note that methods
try_parse_hdrvalcan be assumed to be private as they are not documented in FITSIO doc. and are only called in the fileheader.jl. These changes should not affect the end users.Future improvements
Methods
fits_try_read_keys,fits_try_read_extnameandfits_try_read_extvermay be assumed to be public. Their API shall be clearly documented, defined and fixed. They should be directly tested inruntests.jl.The type of the value of a FITS keyword can be inferred from the first non-space character (a quote
'\''must be a string, a letter'T'or'F'must be a boolean, only spaces must be an empty value and anything else can only be an integer or a floating-point). This fact can be used to speed-up the parsing of FITS keywords inparse_header_val.According to FITS standard Section 5.4.2.6 "Extension Keywords" (https://archive.stsci.edu/fits/fits_standard), EXTNAME shall be a string while EXTVER shall be an integer. Both should not appear in the primary header. This may be taken into account by methods
fits_try_read_extnameandfits_try_read_extver.