astrogo / fitsio

fitsio is a pure-Go package to read and write `FITS` files
BSD 3-Clause "New" or "Revised" License
53 stars 24 forks source link

fitsio: systematically use io.ReadFull for reading padding bytes in decoding #37

Closed airnandez closed 6 years ago

airnandez commented 6 years ago

Avoid redundant checks of the length of data read when using io.ReadFull and use ioutil.Discard when reading the padding bytes, when decoding an image or table, so to avoid allocating a buffer.

No new tests written. All existing tests pass.

airnandez commented 6 years ago

Done both modifying the commit and submitted PR to licence repo.

airnandez commented 6 years ago

When you merge this PR to the lazy-image-loader branch, would you also be willing to merge that branch to master? In that way, when one does go get the improvements included in lazy-image-loader get installed and available.

There are still some API improvements that would be nice to have, but in my opinion they should go into another dedicated branch.

sbinet commented 6 years ago

thanks, but you changed the PR title (that's ok), I was talking about the commit message :)

sbinet commented 6 years ago

wrt the lazy-image-loader branch, I still need to implement the proper split of HDU into a HU and DU part. I am still officially on holidays but I'll give a stab next week.

airnandez commented 6 years ago

Understood about lazy-image-loader.

Regarding the commit message, my (admittedly) limited understanding of git is that it is not possible to change the commit message without changing the commit hash, which may have have consequences with this PR. I may be wrong, of course !

Is there a clean way to change the commit message I'm not aware of?

sbinet commented 6 years ago

your understanding of git is correct, this will change the commit hash :)

but if you push the new commit into this branch, the PR should be correctly updated. (and I'll cherry pick it in master.)

airnandez commented 6 years ago

Done. Thanks for the tip.

sbinet commented 6 years ago

ok... this was a bit convoluted (I prefer to have a rather linear git history) so I cherry picked your change and collected it as a1d964b.

thanks a lot!