Closed emmt closed 5 years ago
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 :wink:
Yes I will do that. Thanks!
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_hdrval
methods: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 typeT
ornothing
if unsuccessful. This follows the behavior of thetryparse
methods 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_hdrval
can 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_extname
andfits_try_read_extver
may 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_extname
andfits_try_read_extver
.