-
Notifications
You must be signed in to change notification settings - Fork 197
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
Add meaningful "representation" formatter to object classes #432
Add meaningful "representation" formatter to object classes #432
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.
Style nit.
Style nit updated. Thanks @erlend-aasland . <-- I learned a new py formatter today. Awesome 🥳 |
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.
Oh, I forgot you can use the #
format specifier to get the 0x
prefix! It won't save any characters, but It saves you one character, and it is slightly neater.
Yes, I am aware. Muscle memory is an odd thing. I worked on a project where we needed "0xABCD" and |
Right, because But, we're way off-topic for this PR. The change looks good with or without the |
I still think |
It is, isn't it? |
Ah, you're correct; I misremembered it as being an int. Sorry for the noise! |
return f"<{type(self).__qualname__} {self.qualname!r} at 0x{self.index:04X}{suffix}>" | ||
|
||
@property | ||
def qualname(self) -> str: |
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.
Please add a docstring, as it is not obvious what qualname
means and how it differs from name
.
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.
Done. Updated.
-- Having qualname
on this object only hurts my eye a little bit, but its due to the special naming regime that pertains to this object only. Adding qualname
to all the others will just be a do-nothing object for the purpose of duck typing.
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'm okay with having it only here. It's purely for internal use right now.
@@ -194,6 +194,9 @@ def __init__(self, pdo_node, com_record, map_array): | |||
self.is_received: bool = False | |||
self._task = None | |||
|
|||
def __repr__(self) -> str: | |||
return f"<{type(self).__qualname__} {self.name!r} at COB-ID 0x{self.cob_id:X}>" |
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.
Why are you formatting with !r
modifier? The attribute is already a simple string, no?
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.
@erlend-aasland perhaps you can chime in why !r
is suitable here? I saw it as an opportunity to avoid using quotes in the 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.
For strings, the !r
add quotes. If you don't want quotes, just remove !r
. If you prefer the explicit style of adding quotes, go ahead with that; it's just a style preference.
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.
To clarify:
>>> s = "thing"
>>> f"'{s}'"
"'thing'"
>>> f"{s!r}"
"'thing'"
>>> f"'{s}'" == f"{s!r}"
True
A common usage pattern is to work with object classes, which include
*Variable()
,*Array()
and*Record()
. Currently the object is printed without any more useful info that its address. This PR adds a richer output for the objects (albeit without address) making it easier to develop. It prints the object name and the index and subindex(The main motivation for PR #431 was to be able to print
<PdoMap...
and not just<Map...
)While writing this PR I noticed an interesting inconsistency in
*Variable()
. Theobj.name
attribute inSdoVariable()
andPdoVariable()
for subindex variable instances contains their parent names:However,
ODVariable()
, differs from this: It contains onlyBattery Level
and not prefixed by the parent object. It was tempting to alter the behavior forODVariable
and ensure the.name
field is handled equally and consistently across all variants, but I did not. It will break current behavior and it meddles with how OD import/export is made. I chose to implement a new propertyODVariable.qualname
much akin to how Pythons class__qualname__
is used. This attr is used from__repr__()
to achieve consistent printing on all variable variants.PS! This PR builds on #431 , so it will be easier to evaluate the diff after this has been accepted. If #431 is rejected, I will update this PR.