-
Notifications
You must be signed in to change notification settings - Fork 174
Open specwcs reffile with context manager #6160
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
Open specwcs reffile with context manager #6160
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6160 +/- ##
=======================================
Coverage 77.15% 77.15%
=======================================
Files 402 402
Lines 34369 34436 +67
=======================================
+ Hits 26517 26569 +52
- Misses 7852 7867 +15
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report at Codecov.
|
try: | ||
velosys = input_model.meta.wcsinfo.velosys | ||
except AttributeError: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a try/except block here as it will never fail to have this attribute since it is in the wcsinfo schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Open specwcs reffile with context manager * Update CHANGES.rst * Address comments
This PR fixes a problem we saw in a unit test where the
specwcs
reffile was incompletely downloaded from CRDS and soasdf.open()
failed. When it failed, it was not closed, as it never went through__exit__
.Not sure if this is a problem with thewith
context manager inasdf.open()
, i.e. is heavy-lifting happening inAsdfFile.__init__
that should be happening inAsdfFile.__enter__
? Not sure.Regardless, in this case, as soon asasdf.open(file)
was called, it opens it without using__enter__
. Is this a bug or feature inasdf
@eslavich ? Or just a small bug here.Separate issue filed in asdf-format/asdf#1006
Also cleaned up a try/except block below.
velosys
is guaranteed to be an attribute as it is in the schema.Checklist