-
Notifications
You must be signed in to change notification settings - Fork 85
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
Don't hard-code class names in __repr__ implementations #1335
Conversation
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.
nit-picky comment : the code might look cleaner if we can use f-strings.
traits/trait_dict_object.py
Outdated
return ( | ||
"{event.__class__.__name__}(" | ||
"removed={event.removed!r}, " | ||
"added={event.added!r}, " | ||
"changed={event.changed!r}" | ||
")".format(event=self) |
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.
nit : this might look cleaner if we use an f-string.
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.
Agreed; and the event
-> self
switch confused me the first time I looked at this code - just use self
throughout.
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.
Agreed; and the
event
->self
switch confused me the first time I looked at this code - just useself
throughout.
I had originally been following what was done here:
traits/traits/observation/_dict_change_event.py
Lines 45 to 52 in 7f8546d
def __repr__(self): | |
return ( | |
"{event.__class__.__name__}(" | |
"object={event.object!r}, " | |
"removed={event.removed!r}, " | |
"added={event.added!r}" | |
")".format(event=self) | |
) |
and with the other
_something_change_event
reprs. Shall I change these to just use self throughout and f strings as well?
Edit: I went ahead and just did this
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 see. Not sure it's worth changing that existing code, given that it's already working. Maybe do that in a separate PR?
Edit: Okay, fine!
@@ -93,6 +93,13 @@ def test_defaults(self): | |||
self.assertEqual(event.removed, []) | |||
self.assertEqual(event.added, []) | |||
|
|||
def test_trait_list_event_str_representation(self): |
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.
How about adding a test for a TraitListEvent
subclass, to check that it's the subclass name that appears in the repr
rather than TraitListEvent
. (That's the original motivation for the change, after all.)
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. Not sure if @mdickinson also wants to rereview.
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.
LGTM2!
fixes #1333
This PR simply updates repr implementations in a few classes to use
self.__class__.__name__
rather than hardcoding the class name, and adds simple tests. Namely, it updates theTraitListEvent
,TraitDictEvent
, andTraitSetEvent
classes as well as theAdaptationOffer
andWeakIDDict
classes.Checklist
- [ ] Update API reference (docs/source/traits_api_reference
)- [ ] Update User manual (docs/source/traits_user_manual
)- [ ] Update type annotation hints intraits-stubs