Clarify ODVariable.data_type cannot be None

[acolomb]

Use a simple integer zero as dummy value for "unset" in instances. This fixes some type checker errors. Add type hints for functions involving those values.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[bizfsc]

Thanks for the PR – removing Optional[int] from data_type is the right direction. Two things to address:

---

1. Use a named constant instead of bare 0

Using not self.data_type is an implicit truthiness test and not self-explanatory. Since 0x00 is unassigned in the CANopen standard (valid types start at BOOLEAN = 0x1), it works semantically – but it should get a name.

Suggested addition in canopen/objectdictionary/datatypes.py:

UNSET = 0x00      # reserved / not a valid CANopen data type
BOOLEAN = 0x1
...

Then in ODVariable.__init__:

self.data_type: int = datatypes.UNSET

And in encode_raw:

elif self.data_type == datatypes.UNSET:
    raise ObjectDictionaryError("Data type has not been specified")

This makes the intent explicit everywhere, including for external callers and subclasses that want to check whether a variable has been configured.

---

2. Missing test coverage for the unset branch

Codecov reports the new elif not self.data_type: branch as uncovered. A minimal test in test/test_od.py would close this gap:

def test_encode_raw_unset_data_type(self):
    from canopen.objectdictionary import ObjectDictionaryError
    var = od.ODVariable("Test unset", 0x1000)
    # data_type intentionally left at default (unset)
    with self.assertRaises(ObjectDictionaryError):
        var.encode_raw(42)

---

The type annotations added to _calc_bit_length, _convert_variable, and _revert_variable in eds.py are fine as-is.

[acolomb]

That AI is a bit overeager ;-)

In this case I think the zero literal is just fine, anything else would be overkill. Simply not var.data_type is pretty self-explanatory, however that thruth value is represented. Let's not over-engineer this.

Fixed the one-line coverage issue, though I question the cost / benefit ratio here.

[acolomb]

What does Codecov mean with "partial"?

[bizfsc]

That AI is a bit overeager ;-)

True, programmed to be a people pleaser, but I just let it (copilot opus) post the review (via github cli), the original "problem" was discovered by myself:

elif not self.data_type:

Is True for 0, but also for None, "", [], False

So we should write

self.data_type == 0

But Naming it leads to good readable code:

if self.data_type is datatypes.UNSET:

That was my only intention.

[acolomb]

Well I don't really care whether it is 0 or [] or None. Only ints are allowed, which can be "enforced" using a static type checker and our type hint. If anyone does assign None to it, that's their fault and completely useless after all.

As for checking, the data_type == 0 approach may raise TypeError at runtime for non-integers, while the current approach simply skips over it. I find the latter more appealing, any further checks should be done on the static typing level. They'll never happen anyway, who sets this attribute explicitly to something but an integer literal?

EDIT: Having said that, of course I agree to avoid magic numbers. That's what makes the truth test appealing here. And IMHO, zero is a "special", not-so-magic number ;-)

[bizfsc]

I still think a named datatype.UNSET is nicer, than plain 0. But this PR is merge ready.