-
Notifications
You must be signed in to change notification settings - Fork 74
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
fix reference to plugins in metadata #2826
Conversation
* changed to be the human-friendly name in spacetelescope#2680, but missed updating a few cases, resulting in moment maps not being linked correctly
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2826 +/- ##
==========================================
- Coverage 88.88% 88.88% -0.01%
==========================================
Files 111 111
Lines 16983 16978 -5
==========================================
- Hits 15096 15091 -5
Misses 1887 1887 ☔ View full report in Codecov by Sentry. |
as regression test. This check currently fails on main but should be passing as part of PR 2826
@@ -128,6 +128,10 @@ def test_moment_calculation(cubeviz_helper, spectrum1d_cube, tmp_path): | |||
assert len(dc.links) == 22 | |||
assert len(dc.external_links) == 4 # pixel linked | |||
# Link 3D z to 2D x and 3D y to 2D y | |||
assert (dc.external_links[0].cids1[0].label == "Pixel Axis 0 [z]" and |
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 confirmed locally that this test fails on main
without this patch as follows:
E AssertionError: assert ('Pixel Axis 0 [z]' == 'Pixel Axis 0 [z]'
E
E Pixel Axis 0 [z] and 'Pixel Axis 0 [y]' == 'Pixel Axis 1 [x]'
E
E - Pixel Axis 1 [x]
E ? ^ ^
E + Pixel Axis 0 [y]
E ? ^ ^)
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.
Wow, the fix is very subtle. Thanks for hunting it down. I added a check as regression test. I think this is good to merge once CI is green. No need for second review.
Thanks for tracking down the offending PR and adding the test! 🎉 |
Description
This pull request fixes internal references to plugin names which changed to be the human-friendly name in #2680 (not yet released), but missed updating a few cases, resulting in moment maps not being linked correctly.
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.