New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error handling in io.fits.compression module #4585

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@anchitjain1234
Contributor

anchitjain1234 commented Feb 8, 2016

For #3118.

Handled 2 errors.
1. When calloc fails.
2. If ZNAXIS keyword is not present in header.
@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 8, 2016

Contributor

@embray Is this correct? Also, I am not sure how to test these changes.

Contributor

anchitjain1234 commented Feb 8, 2016

@embray Is this correct? Also, I am not sure how to test these changes.

@astrofrog astrofrog added the io.fits label Feb 9, 2016

@embray

This comment has been minimized.

Show comment
Hide comment
@embray

embray Feb 17, 2016

Member

@anchitjain1234 Thanks for the fix--I don't have time to review this right now but ping me in a few days or to remind me.

Member

embray commented Feb 17, 2016

@anchitjain1234 Thanks for the fix--I don't have time to review this right now but ping me in a few days or to remind me.

@eteq

This comment has been minimized.

Show comment
Hide comment
@eteq

eteq Apr 5, 2016

Member

@embray - do you think this looks good? The tests are passing and this looks right to me, but I admit I don't understand the underlying origin of #3118 ...

Member

eteq commented Apr 5, 2016

@embray - do you think this looks good? The tests are passing and this looks right to me, but I admit I don't understand the underlying origin of #3118 ...

@astrofrog

This comment has been minimized.

Show comment
Hide comment
@astrofrog

astrofrog May 10, 2016

Member

@embray - this is a ping :) Could you briefly review this?

Member

astrofrog commented May 10, 2016

@embray - this is a ping :) Could you briefly review this?

@pllim

This comment has been minimized.

Show comment
Hide comment
@pllim

pllim Dec 2, 2016

Member

I was hoping to squeeze this into 1.3. It is missing a change log but I cannot take over because this was opened before the new GitHub feature. Ah, well!

The changes (adding some checks and error messages) seem uncontroversial to me, but I am not familiar with the C module. Anyone wants to have another look?

Member

pllim commented Dec 2, 2016

I was hoping to squeeze this into 1.3. It is missing a change log but I cannot take over because this was opened before the new GitHub feature. Ah, well!

The changes (adding some checks and error messages) seem uncontroversial to me, but I am not familiar with the C module. Anyone wants to have another look?

@astrofrog

This comment has been minimized.

Show comment
Hide comment
@astrofrog

astrofrog Dec 2, 2016

Member

@pllim - I'm not familiar enough with the C code to review this. If this does get merged we can just add a changelog entry directly in master.

Member

astrofrog commented Dec 2, 2016

@pllim - I'm not familiar enough with the C code to review this. If this does get merged we can just add a changelog entry directly in master.

@mhvk

This comment has been minimized.

Show comment
Hide comment
@mhvk

mhvk Feb 7, 2017

Contributor

Closing in favour of #5775, which builds on this.

Contributor

mhvk commented Feb 7, 2017

Closing in favour of #5775, which builds on this.

@mhvk mhvk closed this Feb 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment