-
Notifications
You must be signed in to change notification settings - Fork 76
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
deprecate conf-specific helper methods in specviz, specviz2d, cubeviz #3388
base: main
Are you sure you want to change the base?
deprecate conf-specific helper methods in specviz, specviz2d, cubeviz #3388
Conversation
f3902d6
to
bf6e860
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3388 +/- ##
==========================================
- Coverage 88.01% 87.53% -0.49%
==========================================
Files 127 128 +1
Lines 19747 19884 +137
==========================================
+ Hits 17381 17406 +25
- Misses 2366 2478 +112 ☔ View full report in Codecov by Sentry. |
jdaviz/configs/specviz2d/helper.py
Outdated
@@ -152,6 +154,7 @@ def load_data(self, spectrum_2d=None, spectrum_1d=None, spectrum_1d_label=None, | |||
timeout=timeout | |||
) | |||
|
|||
@deprecated(since="4.2") |
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 alternative?
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.
ah, I had meant to either move this logic or postpone it. I'll revisit if its feasible to move to the plugin here or better as a follow-up effort (in which case I'll remove the deprecation for now).
I'll move back to draft for now.
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 think ultimately this needs to be absorbed into the (future) general load_data
, so I'll make a note on that ticket and hold off on deprecating until that's in place.
Just a few nitpicks on wordings. If no more new deprecation you want to add, maybe it is ready to be taken back out of draft after a rebase to pick up CI fix. |
(undeprecated for now to see if existing tests pass)
This reverts commit 6dcf821.
3f827e2
to
d3d5033
Compare
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!
Description
This pull request deprecates config-specific helper methods in specviz, specviz2d, and cubeviz in preparation for deconfigging efforts.
Note that this intentionally defers deprecating:
load_trace
(requiresload_data
support).specviz
access (requires general parser logic)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.