Skip to content
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

Merged

Conversation

sveinse
Copy link
Contributor

@sveinse sveinse commented May 14, 2024

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

# Currently
<ODVariable object at 0x0000023929209B20>

# New OD
<ODVariable 'Device Type' at 0x1000>   # No subindex
<ODVariable 'RTC.DateTime' at 0x2020:01>   # With subindex
<ODArray 'Power bus enabled' at 0x2012>
<ODRecord 'LED' at 0x2030>

# New SDO
<SdoVariable 'Device Type' at 0x1000>
<SdoArray 'Power bus enabled' at 0x2012>
<SdoRecord 'LED' at 0x2030>

# New PDO
<PdoMap 'RxPDO1_node1' at COB-ID 0x513>
<PdoVariable 'RTC.BCD DateTime' at 0x2020:02>

(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(). The obj.name attribute in SdoVariable() and PdoVariable() for subindex variable instances contains their parent names:

Battery.Battery Level   # Example object name var.name

However, ODVariable(), differs from this: It contains only Battery Level and not prefixed by the parent object. It was tempting to alter the behavior for ODVariable 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 property ODVariable.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.

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit.

canopen/objectdictionary/__init__.py Outdated Show resolved Hide resolved
canopen/objectdictionary/__init__.py Outdated Show resolved Hide resolved
canopen/objectdictionary/__init__.py Outdated Show resolved Hide resolved
@sveinse
Copy link
Contributor Author

sveinse commented May 14, 2024

Style nit updated. Thanks @erlend-aasland . <-- I learned a new py formatter today. Awesome 🥳

Copy link
Contributor

@erlend-aasland erlend-aasland left a 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.

canopen/objectdictionary/__init__.py Outdated Show resolved Hide resolved
canopen/objectdictionary/__init__.py Outdated Show resolved Hide resolved
canopen/objectdictionary/__init__.py Outdated Show resolved Hide resolved
canopen/sdo/base.py Outdated Show resolved Hide resolved
canopen/sdo/base.py Outdated Show resolved Hide resolved
canopen/variable.py Outdated Show resolved Hide resolved
@sveinse
Copy link
Contributor Author

sveinse commented May 14, 2024

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 {x:#04X} can't provide that so we had to do it with 0x{x:04X}. And now much later it's stuck 🌞

@erlend-aasland
Copy link
Contributor

I worked on a project where we needed "0xABCD" and {x:#04X} can't provide that so we had to do it with 0x{x:04X}.

Right, because #04X would give you the 0X prefix instead of 0x. That's ... irritating 😄 Well, it's too late to change that in Python; it would be a breaking change. Somewhere in the world, there is production code that depends on that exact format being produced 🤷

But, we're way off-topic for this PR. The change looks good with or without the # specifier. (I added one more remark regarding suffix in #432 (comment).)

@erlend-aasland
Copy link
Contributor

I still think suffix should be represented in hex, not decimal.

@sveinse
Copy link
Contributor Author

sveinse commented May 15, 2024

I still think suffix should be represented in hex, not decimal.

It is, isn't it?

@erlend-aasland
Copy link
Contributor

erlend-aasland commented May 16, 2024

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:
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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}>"
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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

@acolomb acolomb changed the title Feature add repr to object classes Add meaningful "representation" formatter to object classes May 16, 2024
@acolomb acolomb merged commit 3c1d86c into christiansandberg:master May 16, 2024
1 check passed
@sveinse sveinse deleted the feature-add-repr-to-object-classes branch May 16, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants