Memorized DOMAIN ObjectType for a proper export by FedericoSpada · Pull Request #655 · canopen-python/canopen

FedericoSpada · 2026-05-04T20:55:07Z

While working with an EDS that had DOMAIN objects, I've noted that when imported and exported using this library, all DOMAIN Objects were converted to normal Variables.

While importing an EDS, I've changed to save whether a Variable was created from an object with ObjectType==DOMAIN. In this way, it is possible to export it unchanged with the proper ObjectType value.

Changed to save whether a Variable has ObjectType==DOMAIN while importing an EDS, so that it is possible to export it unchanged.

bizfsc

Looking good. As far as I can check it, no new problems introduced for mypy.

friederschueler

Good fix for a real round-trip data-loss bug. The overall approach — adding is_domain on import and using it on export — is clean and well-tested.

A few small points:

Two places in eds.py have missing spaces around == inside a keyword argument (see inline comments).
The new boolean assertions in the tests would be more idiomatic as assertFalse/assertTrue.
Worth considering storing the full object_type integer instead of just a boolean, but that is a larger API change and not required for this fix.

friederschueler · 2026-05-06T13:26:56Z


            if object_type in (objectcodes.VAR, objectcodes.DOMAIN):
-                var = build_variable(eds, section, node_id, index)
+                var = build_variable(eds, section, node_id, index, is_domain=object_type==objectcodes.DOMAIN)


PEP 8: missing spaces around the == comparison inside the keyword argument.

Suggested change

var = build_variable(eds, section, node_id, index, is_domain=object_type==objectcodes.DOMAIN)

var = build_variable(eds, section, node_id, index, is_domain=object_type == objectcodes.DOMAIN)

I've changed it, but for me, it is incredibly misleading: you are giving "priority" to the assignment instead of the operation that is performed before.
A possible solution would be is_domain=(object_type == objectcodes.DOMAIN), but Pylint complains about the unnecessary parentheses.
For me, the proper way to write it would be is_domain = object_type==objectcodes.DOMAIN.
I've never understood why PEP 8 requires no spaces for "=" in function invocations...

friederschueler · 2026-05-06T13:27:03Z

+                    object_type = int(eds.get(section, "ObjectType"), 0)
+                except NoOptionError:
+                    object_type = objectcodes.VAR
+                var = build_variable(eds, section, node_id, index, subindex, is_domain=object_type==objectcodes.DOMAIN)


Same PEP 8 spacing issue as above.

Suggested change

var = build_variable(eds, section, node_id, index, subindex, is_domain=object_type==objectcodes.DOMAIN)

var = build_variable(eds, section, node_id, index, subindex, is_domain=object_type == objectcodes.DOMAIN)

friederschueler · 2026-05-06T13:27:12Z

        self.assertEqual(var.name, 'Producer heartbeat time')
        self.assertEqual(var.data_type, canopen.objectdictionary.UNSIGNED16)
        self.assertEqual(var.access_type, 'rw')
+        self.assertEqual(var.is_domain, False)


Nit: for boolean checks the unittest idiom is assertFalse/assertTrue rather than assertEqual(..., False/True). Same pattern applies to the other four analogous assertions added in this PR.

Suggested change

self.assertEqual(var.is_domain, False)

self.assertFalse(var.is_domain)

codecov · 2026-05-06T20:04:28Z

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
canopen/objectdictionary/eds.py	80.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

acolomb · 2026-05-07T07:44:27Z

@friederschueler please use the "Squash and merge" GitHub function for PRs, with a sensible, hand-checked commit message.

In this case, I hadn't even got around to looking at the PR contents, so I'd like a bit more time before you merge future contributions. Thank you! 🙂

friederschueler · 2026-05-07T08:07:58Z

@acolomb I am sorry, this was muscle memory from work. I forgot we use hand-crafted messages. I personally think that the automated PR merge messages are just fine, because the PR should be named in a good way. Also isn't squash commit the default? I will also give you more time to review.

acolomb · 2026-05-07T08:05:27Z

+                    object_type = int(eds.get(section, "ObjectType"), 0)
+                except NoOptionError:
+                    object_type = objectcodes.VAR
+                var = build_variable(eds, section, node_id, index, subindex, is_domain=object_type == objectcodes.DOMAIN)


This is certainly beyond a sensible line length limit. ;-)

I've codified 96 chars as a guideline in #660, please try to stick to that in future contributions.

acolomb · 2026-05-07T08:06:07Z


            if object_type in (objectcodes.VAR, objectcodes.DOMAIN):
-                var = build_variable(eds, section, node_id, index)
+                var = build_variable(eds, section, node_id, index, is_domain=object_type == objectcodes.DOMAIN)


Line length, see below.

acolomb · 2026-05-07T08:10:24Z



-def build_variable(eds, section, node_id, index, subindex=0):
+def build_variable(eds, section, node_id, index, subindex=0, is_domain=False):


Frankly I don't particularly like this function signature. The is_domain attribute could be set directly on the resulting object instead of adding a new call parameter.

Or even better, pass the whole object_code value and have the logic for the boolean attribute here in this function. That's why we have this function with centralized logic.

Implemented the management for DOMAIN Objects

f1c01e4

Changed to save whether a Variable has ObjectType==DOMAIN while importing an EDS, so that it is possible to export it unchanged.

bizfsc approved these changes May 5, 2026

View reviewed changes

friederschueler reviewed May 6, 2026

View reviewed changes

Small changes requested by the maintainers

aaa495e

friederschueler self-requested a review May 6, 2026 19:56

friederschueler approved these changes May 6, 2026

View reviewed changes

friederschueler merged commit b73daa6 into canopen-python:master May 6, 2026
4 of 5 checks passed

FedericoSpada deleted the export_domain_objects branch May 6, 2026 20:08

acolomb reviewed May 7, 2026

View reviewed changes

acolomb mentioned this pull request May 7, 2026

eds: Style and type fixes around build_variable() #661

Merged

	var = build_variable(eds, section, node_id, index, is_domain=object_type==objectcodes.DOMAIN)
	var = build_variable(eds, section, node_id, index, is_domain=object_type == objectcodes.DOMAIN)

	self.assertEqual(var.is_domain, False)
	self.assertFalse(var.is_domain)



		def build_variable(eds, section, node_id, index, subindex=0):
		def build_variable(eds, section, node_id, index, subindex=0, is_domain=False):

Conversation

FedericoSpada commented May 4, 2026

Uh oh!

bizfsc left a comment

Choose a reason for hiding this comment

Uh oh!

friederschueler left a comment

Choose a reason for hiding this comment

Uh oh!

friederschueler May 6, 2026

Choose a reason for hiding this comment

Uh oh!

FedericoSpada May 6, 2026

Choose a reason for hiding this comment

Uh oh!

friederschueler May 6, 2026

Choose a reason for hiding this comment

Uh oh!

friederschueler May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented May 6, 2026

Codecov Report

Uh oh!

Uh oh!

acolomb commented May 7, 2026

Uh oh!

friederschueler commented May 7, 2026

Uh oh!

acolomb May 7, 2026

Choose a reason for hiding this comment

Uh oh!

acolomb May 7, 2026

Choose a reason for hiding this comment

Uh oh!

acolomb May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants