Skip to content
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

Better handling of AsdfInFits #241

Merged
merged 9 commits into from
Jun 14, 2017
Merged

Better handling of AsdfInFits #241

merged 9 commits into from
Jun 14, 2017

Conversation

drdavella
Copy link
Contributor

@drdavella drdavella commented Jun 2, 2017

Issue #182 proposed more intelligent handling of FITS files with ASDF extensions. This branch allows AsdfInFits.open to handle URIs, open file handles, and HDULists, whereas previously it only handled HDULists. Also, AsdfFile.open now attempts to handle FITS files with ASDF extensions if the given file-like object is not obviously ASDF. Comments on implementation, code style, documentation, etc. welcome.

@drdavella drdavella self-assigned this Jun 2, 2017
The previous commit introduced a bug in the _open_impl method of AsdfFile.
This commit fixes that bug and changes the use of kwargs in that method
back to an explicit argument list.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 93.83% when pulling c900d69 on asdf-in-fits into 4a43822 on master.

@drdavella drdavella removed the request for review from embray June 5, 2017 20:24
@perrygreenfield
Copy link
Contributor

Looks reasonable to me.

@nden nden added this to the 1.2.2 milestone Jun 8, 2017
@nden
Copy link
Contributor

nden commented Jun 8, 2017

@drdavella This needs a change log entry.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 93.83% when pulling c342eed on asdf-in-fits into 4a43822 on master.

@nden
Copy link
Contributor

nden commented Jun 9, 2017

@drdavella I forgot to mention that we have adopted the convention to use the pull request number in CHANGES.rst, not the issue number.

Could you add a test case and perhaps an example in the docs. I am currently getting an error trying this with a fits file but my setup may be the problem.

@drdavella
Copy link
Contributor Author

I added an issue (#246) to address the bug you were seeing with missing extensions.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 93.792% when pulling f4519f2 on asdf-in-fits into 4a43822 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 94.032% when pulling dd93352 on asdf-in-fits into 4a43822 on master.

Copy link
Contributor

@nden nden left a comment

Choose a reason for hiding this comment

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

LGTM

@drdavella drdavella merged commit 2e15146 into master Jun 14, 2017
@drdavella drdavella deleted the asdf-in-fits branch June 14, 2017 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants