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

a few fixes #26

Closed emmt closed 9 years ago

emmt commented 9 years ago

A few changes to:

kbarbary commented 9 years ago

Can you rebase your branch on the current master? After merging #25, the parse_header_val function has changed. You'll have to resolve the conflicts in that function. (rebase is preferable to merge in this case!).

This looks good except I'm not in favor of having readkey and readheader work on the low-level interface. I think we should eventually move away from the low-level API and instead try to ensure that the high-level interface suits everyone's needs. So, I'd rather not add new functionality unless it is just a thin wrapper for cfitsio like the rest of the low-level API. Since this discussion goes beyond this PR, I've opened a new issue for it: #27.

emmt commented 9 years ago

We have rebased, the current pull request should be OK the access of readheader/readkey at lowlevel have been removed as you suggested

Le 08/04/2015 18:01, Kyle Barbary a écrit :

Can you rebase your branch on the current master? After merging #25 https://github.com/JuliaAstro/FITSIO.jl/pull/25, the |parse_header_val| function has changed. You'll have to resolve the conflicts in that function. (rebase is preferable to merge in this case!).

This looks good except I'm not in favor of having |readkey| and |readheader| work on the low-level interface. I think we should eventually move away from the low-level API and instead try to ensure that the high-level interface suits everyone's needs. So, I'd rather not add new functionality unless it is just a thin wrapper for cfitsio like the rest of the low-level API. Since this discussion goes beyond this PR, I've opened a new issue for it: #27 https://github.com/JuliaAstro/FITSIO.jl/issues/27.

— Reply to this email directly or view it on GitHub https://github.com/JuliaAstro/FITSIO.jl/pull/26#issuecomment-90959953.

kbarbary commented 9 years ago

Looks good! Merging...

For future reference, your rebase seems to have gone a bit wonky - there's multiple commits on this branch with the same comment (though the end result is fine). If it helps, I've written down a very short guide to rebase here ("what happens when the upstream branch is updated?").

emmt commented 9 years ago

Thank you for pointing this link. I am not yet very comfortable with Git and GitHub principles...

Looks good! Merging...

For future reference, your rebase seems to have gone a bit wonky - there's multiple commits on this branch with the same comment (though the end result is fine). If it helps, I've written down a very short guide to rebase here http://sncosmo.readthedocs.org/en/v1.0.x/contributing.html#what-happens-when-the-upstream-branch-is-updated ("what happens when the upstream branch is updated?").