user API for metadata viewer plugin #1634
Conversation
Codecov ReportBase: 86.33% // Head: 86.34% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1634 +/- ##
=======================================
Coverage 86.33% 86.34%
=======================================
Files 95 95
Lines 9618 9622 +4
=======================================
+ Hits 8304 8308 +4
Misses 1314 1314
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
kecnry
requested review from
duytnguyendtn,
rosteen,
javerbukh,
ojustino and
pllim
as code owners
| @@ -2,13 +2,28 @@ | |||
|
|
|||
| from jdaviz.core.registries import tray_registry | |||
| from jdaviz.core.template_mixin import PluginTemplateMixin, DatasetSelectMixin | |||
| from jdaviz.core.user_api import PluginUserApi | |||
| from jdaviz.utils import PRIHDR_KEY, COMMENTCARD_KEY | |||
|
|
|||
| __all__ = ['MetadataViewer'] | |||
|
|
|||
|
|
|||
| @tray_registry('g-metadata-viewer', label="Metadata Viewer") | |||
There was a problem hiding this comment.
should we consider changing the label to just "Metadata"?
There was a problem hiding this comment.
I'm not sure about just "Metadata" but I agree "Metadata Viewer" is worse. Maybe "Metadata Plugin"?
There was a problem hiding this comment.
If you remove "Viewer," they might get the wrong idea that they can edit the metadata with this plugin. In IRAF, you could use a task to edit FITS header, so...
There was a problem hiding this comment.
if I switch exposing the metadata traitlet to the read-only infrastructure added in #1643, then maybe that and some docs updates would avoid that confusion? Is there any chance we'll ever extend this to allow editing metadata (either from the UI or API)?
There was a problem hiding this comment.
I can see editing metadata as a common use case and worth supporting. Implementing it on the other hand would be a...difficult task.
There was a problem hiding this comment.
Let's not open that can of worms here. Can you imagine if someone accidentally overwrites a keyword that is important for WCS transformation?
There was a problem hiding this comment.
I did change it so that plugin.metadata is read-only from the user API (otherwise they could change the traitlet and the plugin would update to show that, but it would not persist when changing the data dropdown, etc).
There was a problem hiding this comment.
With the readonly plugin.metadata is anyone opposed to removing "Viewer"? I just think viz.plugins['Metadata'] and having "Metadata" in the plugin tray menu feels simpler and more natural, as long as we can sufficiently avoid user-confusion through the API and docs.
There was a problem hiding this comment.
Yeah, that is a good compromise. Thanks!
|
More fundamentally, does this really need any public API? This basically exposes what is already in |
Perhaps not, but the intent is to provide consistent access to all the plugins and in a way that mirrors the UI functionality so that users can more easily transition from the UI to scripting in the notebook. |
kecnry
force-pushed
the
api-metadata-viewer
branch
from
3f8ac9d to
6b5b5f3
Compare
jdaviz/configs/default/plugins/metadata_viewer/metadata_viewer.py
Outdated
Show resolved
Hide resolved
|
|
||
| * :meth:`~jdaviz.core.template_mixin.PluginTemplateMixin.show` | ||
| * :meth:`~jdaviz.core.template_mixin.PluginTemplateMixin.open_in_tray` | ||
| * ``dataset`` (:class:`~jdaviz.core.template_mixin.DatasetSelect`): |
There was a problem hiding this comment.
Hmm. Should we change this to "data_label" to be consistent with another plugin?
There was a problem hiding this comment.
I don't think so. In this case, it is a selectable dropdown, as opposed to a read-only indication of the top-layer. If anything, I'd be in favor of changing other instances of data_label to dataset and have it just show as a single choice. That then gives us flexibility to change it to a proper dropdown in the future.
kecnry
force-pushed
the
api-metadata-viewer
branch
from
6b5b5f3 to
393fe6c
Compare
Co-authored-by: P. L. Lim <[email protected]>
Description
This pull request extends on #1401 and provides user-API access to the metdata viewer plugin.
Plugin User API docs
TODO:
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.
triviallabel.CHANGES.rst?