-
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
allow overwriting existing registry entries #2744
allow overwriting existing registry entries #2744
Conversation
c9e2be8
to
f9c348e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2744 +/- ##
==========================================
+ Coverage 88.85% 88.92% +0.07%
==========================================
Files 108 108
Lines 15920 16003 +83
==========================================
+ Hits 14145 14230 +85
+ Misses 1775 1773 -2 ☔ View full report in Codecov by Sentry. |
@@ -75,10 +77,12 @@ def add(self, name, cls, label=None): | |||
in the first parameter. | |||
label : str, optional | |||
The label displayed in the tooltip when hovering over the tray tab. | |||
overwrite : bool, optional | |||
Whether to overwrite an existing entry with the same ``label``. |
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.
Maybe add one more sentence to explain what would happen to the old thing that was overwritten?
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.
suggestions for wording? It is replacing it in the registry, so calls to access the item will retrieve the new one instead.
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.
Is the old one going to be destroyed somewhere or just sitting in purgatory? I am not familiar with this machinery.
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.
This doesn't do anything to destroy it, no, it just replaces the reference in the registry.
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.
Should we destroy it?
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 see any reason to, its generally just a class and shouldn't be instantiated yet
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.
Code looks like code. Thanks!
It is Friday and the weather is nice, let's get this in. 😆 |
Description
This pull request adds support for overwriting existing registry entries, which is particularly useful for downstream apps that want to modify something already in the registry without having to give it a new name.
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.