-
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
"Imviz Line Profiles (XY)" renamed to "Image Profiles (XY)" #3121
Conversation
if 'Orientation' in plugins: | ||
# renamed in 3.9 | ||
plugins['Links Control'] = plugins['Orientation']._obj.user_api | ||
plugins['Links Control']._deprecation_msg = 'in the future, the formerly named \"Links Control\" plugin will only be available by its new name: \"Orientation\".' # noqa | ||
if 'Canvas Rotation' in plugins: | ||
# removed in 3.9 | ||
plugins['Canvas Rotation']._deprecation_msg = 'this functionality will be removed in favor of the implementation for rotation in the \"Orientation\" plugin.' # noqa | ||
if 'Export' in plugins: | ||
# renamed in 3.9 | ||
plugins['Export Plot'] = plugins['Export']._obj.user_api | ||
plugins['Export Plot']._deprecation_msg = 'in the future, the formerly named \"Export Plot\" plugin will only be available by its new name: \"Export\".' # noqa |
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 these have now been more than 2 releases, so maybe we can remove for 4.0?
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 don't think that belongs in scope here, so I made a note in the 4.0 release ticket)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3121 +/- ##
==========================================
+ Coverage 88.91% 88.93% +0.01%
==========================================
Files 111 112 +1
Lines 17365 17399 +34
==========================================
+ Hits 15440 15473 +33
- Misses 1925 1926 +1 ☔ View full report in Codecov by Sentry. |
Any downstream notebooks need updating to go with this? |
Not that I know, there is currently no API access besides showing/opening the plugin, so I doubt any notebook makes use of this. If it does, they'll start seeing the "deprecation" message and hopefully know how to update before we remove that. |
37b2e30
to
75b0da0
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.
Looks uncontroversial enough. Thanks!
(let's not merge until we have a chance to discuss generalizing the name - just canceled auto-merge) |
Oh OK... I thought the ticket was pretty clear. Sorry if I misunderstood. |
75b0da0
to
e572b1c
Compare
e572b1c
to
2e295ad
Compare
@pllim - I did find one mention in a concept notebook and updated that here along with changing to "Image Profiles (XY)" as discussed offline. Are you ok with marking this as trivial and merging from your previous approval? |
jdaviz/configs/imviz/plugins/line_profile_xy/line_profile_xy.py
Outdated
Show resolved
Hide resolved
2e295ad
to
65adcf1
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 renames "Imviz Line Profiles (XY)" to
"Line Profiles (XY)""Image Profiles (XY)" along with deprecation for API access (although the plugin API itself is still not exposed).If we want to name this to something more general (especially if we would ever want to re-use this for 2d spectra and want to avoid confusion with spectral lines) now would be the time to do so.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.