-
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
Rename Subsets Plugin #3304
Rename Subsets Plugin #3304
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3304 +/- ##
==========================================
- 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:
|
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.
Thanks (and sorry for the whiplash! 😉 )
The app-bar widget should also be removed from the API docs (currently appearing at https://jdaviz--3304.org.readthedocs.build/en/3304/reference/api_plugins.html#module-jdaviz.configs.default.plugins.subset_tools.subset_tools)
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 good! my personal opinion would be to rename configs/default/plugins/subset_tools and subset_tools.py to something more clear like subset_app_bar_widget
That crossed my mind as well, but it felt quite long, so I renamed it to |
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.
was git used to rename the files? For some reason its showing as the old file entirely deleted and a new one created instead of just a rename (but maybe that is because of the path rename as well?)
If possible, it would be nice to maintain the git history of the contents of all these files so that blame, etc, continues to function
Because the contents also changed (e.g., class name). |
Can we do that in two separate commits then to retain history? |
I have an idea to make it happen, and I am currently working on it. |
@kecnry |
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.
Thanks for trying! Looks like it might not be possible with the directory rename, but git may still be able to follow this history with --follow
(for future inspections).
You could try something like this (after merge) but not sure how well it works if you are moving things around. It was nice when we applied https://github.com/astropy/astropy/blob/main/.git-blame-ignore-revs |
7647f68
to
ddf84b3
Compare
@kecnry Thank you for your approval. I have rebased to ensure the |
Description
This pull request
Subsets
back toSubset Tools,
returning it to its original state.Subset Tools
toSubsetAppBarWidget
Fixes #
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.