-
Notifications
You must be signed in to change notification settings - Fork 75
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
Metadata Viewer: API to return dict, not list #3292
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3292 +/- ##
==========================================
- Coverage 88.81% 88.81% -0.01%
==========================================
Files 125 125
Lines 19030 19036 +6
==========================================
+ Hits 16901 16906 +5
- Misses 2129 2130 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
jdaviz/configs/default/plugins/metadata_viewer/metadata_viewer.py
Outdated
Show resolved
Hide resolved
During 2024-11-15 tag-up, it was agreed to deprecate
@camipacifici , this was also discussed at tag-up but unclear if this will be confusing to users or not. Description is only applicable when it comes from FITS header (but even then, not always there). In @kecnry suggested a nested dictionary. But will a nested dictionary be confusing too? Also a nested dictionary cannot be roundtripped back into a FITS Header easily. |
nested dictionary could look something like:
|
Is there somewhere in the code that we do this in Jdaviz that would be made more complicated, or are you talking about a user who might want to put the metadata returned by this back into a Header? |
@pllim I think users are very familiar with what |
Yes, there is always a chance a user did a cube smooth or whatever and then wants to write their own FITS file back out and then want to stuff in header from original cube. |
Okay, looks like I give fits.Header({"a": 1}) This does not work. fits.Header({"a": (1, "aaa")}) But if you do it keyword by keyword, this works. hdr = fits.Header()
hdr["a"] = (1, "aaa") So I guess simple full roundtrip is quite impossible. Just a matter of how much code you want user to write, like this. for key, val in meta.items():
hdr[key] = val Or like this. for key, val in meta.items():
hdr[key] = (val["value"], val.get("description", "")) Any preference? |
I'm in favor of the nested dictionary approach, I think that's the clearest thing to return. We could always provide a |
811f28a
to
7d56136
Compare
jdaviz/configs/default/plugins/metadata_viewer/metadata_viewer.py
Outdated
Show resolved
Hide resolved
@@ -34,6 +35,9 @@ def __getattr__(self, attr): | |||
if attr in _internal_attrs or attr not in self._expose: | |||
return super().__getattribute__(attr) | |||
|
|||
if attr in self._deprecated: | |||
logging.warning("DeprecationWarning: %s is deprecated" % attr) |
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.
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.
I thought about making it fancier so it can say "use meta instead" but decided against it because hopefully we won't do this often and I don't want to overcomplicate the machinery.
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!
Thanks for the reviews! |
Description
This pull request is to address the request, "File metadata extracted from the API is hard to parse. Maybe a dictionary is better?"
I avoided changing
metadata
itself because that would mean messing with the front-end stuff that already works. Also might be subtly confusing because the same attribute now returns something different. Maybe best to have a clean break using a different attribute name.Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.