Skip to content

Fix FITS convenience functions to actually close files. - #4786

Merged
pllim merged 2 commits into
astropy:masterfrom
saimn:fits-resource-warning
Apr 27, 2016
Merged

pllim merged 2 commits into
astropy:masterfrom
saimn:fits-resource-warning

Conversation

@saimn

@saimn saimn commented Apr 18, 2016 •

Copy link
Copy Markdown
Contributor

Ref #2824 for an older bug report. On Python 3 ResourceWarning warnings were raised because the FITS convenience methods (getdata, getheader, append, update) were not closing files as they were supposed to.

According to @embray's comment (#2824 (comment)) this could be due to some changes but I didn't find which one.

This was hidden because ResourceWarning are filtered by default (https://docs.python.org/3.4/library/warnings.html#default-warning-filters), but can be shown by unittest or in my case by nosetest. It seems that they are also hidden with py.test.

Ref astropy#2824 an older bug report. On Python 3 `ResourceWarning` warnings were
raised because the FITS convenience methods (`getdata`, `getheader`, `append`,
`update`) were not closing files as they were supposed to.
@astrofrog

Copy link
Copy Markdown
Member

@embray - could you review this if you have time? Thanks!

@embray embray added the Bug label Apr 20, 2016
@embray embray added this to the v1.1.3 milestone Apr 20, 2016
@embray

embray commented Apr 20, 2016

Copy link
Copy Markdown
Member

Thanks. You can add bug fix changelog entries in v1.1.3 and IMO v1.0.10.

@embray

embray commented Apr 20, 2016

Copy link
Copy Markdown
Member

Once that's done and this is merged you can also close #2824 I think?

@saimn

saimn commented Apr 20, 2016

Copy link
Copy Markdown
Contributor Author

Ok, added the changelog entries.

@saimn

saimn commented Apr 26, 2016

Copy link
Copy Markdown
Contributor Author

Up ?

Comment thread CHANGES.rst Outdated

- ``astropy.io.fits``

- Fix convenience functions (`getdata`, `getheader`, `append`, `update`) to

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be under "bug fix", not "new features", if needed at all. I am not sure why @embray asked for duplicate entries. Usually, it is just under one section...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks ! (and fixed)
Not sure either how it works for bug fixes that affect the 2 previous versions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, please use double backticks in the changelog, otherwise they will at some point turn into sphinx warnings...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah indeed, thanks @bsipocz !

@saimn
saimn force-pushed the fits-resource-warning branch from f3d3dd5 to 7c274d5 Compare April 26, 2016 20:23
@saimn
saimn force-pushed the fits-resource-warning branch from 7c274d5 to 1460986 Compare April 27, 2016 10:22
@embray

embray commented Apr 27, 2016

Copy link
Copy Markdown
Member

As for the duplicate entries, we've been a little inconsistent about that. Right now we don't have a good process--for bug fixes in particular--to mark what versions it should be backported to. Currently the next "highest version" bug fix release is v1.1.3. But if we are in still maintenance mode for an LTS (the latest being v1.0) then really there should be two milestones: 1.1.3 and 1.0.10. I'm actually not 100% sure if we'll still do another bug fix release for v1.0 because I forget whether or not the LTS support has expired (though I would say it defacto does not expire until a new LTS release is out).

So TL;DR wherever possible bug fixes should be backported to both the "current" bugfix release, and the next LTS bugfix release. We don't have a great process in the ticket system right now to indicate that though (I miss Trac). But there should be two changelog entries so that when v1.0.10 is release it's clear at least in its changelog what bugs were fixed.

@pllim pllim modified the milestones: v1.0.10, v1.1.3 Apr 27, 2016
@pllim

pllim commented Apr 27, 2016

Copy link
Copy Markdown
Member

Thanks for the clarification, @embray ! Since it is approved, I am merging.

@pllim
pllim merged commit d3bbc66 into astropy:master Apr 27, 2016
@saimn
saimn deleted the fits-resource-warning branch April 27, 2016 13:54
astrofrog pushed a commit that referenced this pull request May 16, 2016
Fix FITS convenience functions to actually close files.
astrofrog pushed a commit that referenced this pull request May 16, 2016
Fix FITS convenience functions to actually close files.
astrofrog pushed a commit that referenced this pull request May 16, 2016
Fix FITS convenience functions to actually close files.
embray added a commit to embray/astropy that referenced this pull request Jun 10, 2016
this allows reversing most of astropy#4786 which introduced a regression
on how file objects are handled by convenience functions.

This fixes the regression and includes a (partial) regression test.
Also fixes up exception handling around convenience functions a bit.
embray added a commit to embray/astropy that referenced this pull request Jun 10, 2016
this allows reversing most of astropy#4786 which introduced a regression
on how file objects are handled by convenience functions.

This fixes the regression and includes a (partial) regression test.
Also fixes up exception handling around convenience functions a bit.
eteq pushed a commit that referenced this pull request Jun 12, 2016
this allows reversing most of #4786 which introduced a regression
on how file objects are handled by convenience functions.

This fixes the regression and includes a (partial) regression test.
Also fixes up exception handling around convenience functions a bit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants