JuliaAstro / FITSIO.jl

Flexible Image Transport System (FITS) file support for Julia
http://juliaastro.org/FITSIO.jl/
MIT License
55 stars 29 forks source link

correctly capture status then forget it #7

Closed kbarbary closed 10 years ago

kbarbary commented 10 years ago

This removes the status field from FITSFile, instead making status a local variable in each function. In each function, an appropriate error is raised if status != 0, so there should be no need to keep a non-zero status in a FITSFile instance.

Here is an example of why keeping a non-zero status in FITSFile, as is the existing intended behavior (suppose the keyword "EXTNAME" does not exist in the CHDU):

try
    extname = fits_read_keyword(f, "EXTNAME")[1]
catch
end
fits_movabs_hdu(f, i)  # <-- fails with error message about a missing keyword

Even though we're catching the error raised by fits_read_keyword, f.status was set to 202 anyway. When we get to fits_movabs_hdu(f, i), it returns immediately because f.status is nonzero, and returns the error message associated with code 202.

kbarbary commented 10 years ago

Rebased.

Any thoughts, @nolta @ziotom78? Admittedly, I haven't thought that much about whether a FITSFile can get into a really screwed up state where there should be flag in FITSFile that indicates this. However, we have to change something. The current behavior is not the intended behavior, and the intended behavior is not good either (as illustrated by the try... catch example above).

ziotom78 commented 10 years ago

I agree with @kbarbary's point of view: the persistent status variable was probably implemented to imitate the way C code using CFITSIO is usually written. But Julia supports exceptions, and IMHO this is probably the cleanest and sanest thing to do.

(Moreover, this would make FITSIO.jl behave the same as the majority of Julia libraries, where the error status is not persistent. This would follow the POLA.)

nolta commented 10 years ago

Yeah, this seems reasonable.