Skip to content

Ensure proper close after open in export_od #469

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.

Sign up for GitHub

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

Merged
acolomb merged 3 commits into from
Jun 30, 2024
Merged

Ensure proper close after open in export_od #469

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9274b4a
Ensure proper close after open
sveinse Jun 20, 2024
76574cd
Add tests to add code coverage of export_od
sveinse Jun 20, 2024
4364d89
Update canopen/objectdictionary/__init__.py
sveinse Jun 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 27 additions & 23 deletions canopen/objectdictionary/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,29 +29,33 @@ def export_od(od, dest: Union[str, TextIO, None] = None, doc_type: Optional[str]
:rtype: str or None
"""

doctypes = {"eds", "dcf"}
if isinstance(dest, str):
if doc_type is None:
for t in doctypes:
if dest.endswith(f".{t}"):
doc_type = t
break

if doc_type is None:
doc_type = "eds"
dest = open(dest, 'w')
assert doc_type in doctypes

if doc_type == "eds":
from canopen.objectdictionary import eds
return eds.export_eds(od, dest)
elif doc_type == "dcf":
from canopen.objectdictionary import eds
return eds.export_dcf(od, dest)

# If dest is opened in this fn, it should be closed
if type(dest) is str:
dest.close()
opened_here = False
try:
doctypes = {"eds", "dcf"}
if isinstance(dest, str):
if doc_type is None:
for t in doctypes:
if dest.endswith(f".{t}"):
doc_type = t
break

if doc_type is None:
doc_type = "eds"
dest = open(dest, 'w')
opened_here = True

if doc_type == "eds":
from canopen.objectdictionary import eds
return eds.export_eds(od, dest)
elif doc_type == "dcf":
from canopen.objectdictionary import eds
return eds.export_dcf(od, dest)
else:
raise NotImplementedError(f"No support for the {doc_type!r} format")
Comment on lines +52 to +53
Copy link
Contributor

@erlend-aasland erlend-aasland Jul 1, 2024

Choose a reason for hiding this comment

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

Hm, I noticed this a little bit too late... this is a little bit unconventional; NotImplementedError is not meant for such use. IMO, ObjectDictionaryError would be more appropriate.

Copy link
Collaborator Author

@sveinse sveinse Jul 2, 2024

Choose a reason for hiding this comment

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

Right. Agreed. It's not always easy to "just fix" something like the close issue and then uncovering other things that needs fixing too. ;)

Copy link
Contributor

@erlend-aasland erlend-aasland Jul 2, 2024

Choose a reason for hiding this comment

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

Yeah, one fix quickly leads to the discovery of other issues :) I'll create an issue about this.

Copy link
Contributor

@erlend-aasland erlend-aasland Jul 2, 2024

Choose a reason for hiding this comment

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

I created #475.

Copy link
Contributor

@erlend-aasland erlend-aasland Jul 2, 2024

Choose a reason for hiding this comment

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

For the record; before this PR, this check was an assert (IOW, AssertionError, unless running with -O or -OO).

finally:
# If dest is opened in this fn, it should be closed
if opened_here:
dest.close()


def import_od(
Expand Down
142 changes: 81 additions & 61 deletions test/test_eds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,68 +184,88 @@ def test_export_eds(self):
from pathlib import Path
with tempfile.TemporaryDirectory() as tempdir:
for doctype in {"eds", "dcf"}:

# Test export_od with file object
tempfile = str(Path(tempdir, "test." + doctype))
with open(tempfile, "w+") as tempeds:
print(f"exporting {doctype} to {tempeds.name}")
canopen.export_od(self.od, tempeds, doc_type=doctype)

exported_od = canopen.import_od(tempfile)

for index in exported_od:
self.assertIn(exported_od[index].name, self.od)
self.assertIn(index, self.od)

for index in self.od:
if index < 0x0008:
# ignore dummies
continue
self.assertIn(self.od[index].name, exported_od)
self.assertIn(index, exported_od)

actual_object = exported_od[index]
expected_object = self.od[index]
self.assertEqual(type(actual_object), type(expected_object))
self.assertEqual(actual_object.name, expected_object.name)

if isinstance(actual_object, canopen.objectdictionary.ODVariable):
expected_vars = [expected_object]
actual_vars = [actual_object]
else:
expected_vars = [expected_object[idx] for idx in expected_object]
actual_vars = [actual_object[idx] for idx in actual_object]

for prop in [
"allowed_baudrates",
"vendor_name",
"vendor_number",
"product_name",
"product_number",
"revision_number",
"order_code",
"simple_boot_up_master",
"simple_boot_up_slave",
"granularity",
"dynamic_channels_supported",
"group_messaging",
"nr_of_RXPDO",
"nr_of_TXPDO",
"LSS_supported",
]:
self.assertEqual(getattr(self.od.device_information, prop),
getattr(exported_od.device_information, prop),
f"prop {prop!r} mismatch on DeviceInfo")

for evar, avar in zip(expected_vars, actual_vars):
self.assertEqual(getattr(avar, "data_type", None), getattr(evar, "data_type", None),
f" mismatch on {pretty_index(evar.index, evar.subindex)}")
self.assertEqual(getattr(avar, "default_raw", None), getattr(evar, "default_raw", None),
f" mismatch on {pretty_index(evar.index, evar.subindex)}")
self.assertEqual(getattr(avar, "min", None), getattr(evar, "min", None),
f" mismatch on {pretty_index(evar.index, evar.subindex)}")
self.assertEqual(getattr(avar, "max", None), getattr(evar, "max", None),
f" mismatch on {pretty_index(evar.index, evar.subindex)}")
if doctype == "dcf":
self.assertEqual(getattr(avar, "value", None), getattr(evar, "value", None),
f" mismatch on {pretty_index(evar.index, evar.subindex)}")

self.assertEqual(self.od.comments, exported_od.comments)
self.verify_od(tempfile, doctype)

# Test export_od handling opening and closing the file
tempfile = str(Path(tempdir, "test2." + doctype))
canopen.export_od(self.od, tempfile, doc_type=doctype)
self.verify_od(tempfile, doctype)

# Test for unknown doctype
with self.assertRaises(NotImplementedError):
tempfile = str(Path(tempdir, "test.unknown"))
canopen.export_od(self.od, tempfile, doc_type="unknown")

# Omit doctype
tempfile = str(Path(tempdir, "test.eds"))
canopen.export_od(self.od, tempfile)
self.verify_od(tempfile, "eds")


def verify_od(self, tempfile, doctype):
exported_od = canopen.import_od(tempfile)

for index in exported_od:
self.assertIn(exported_od[index].name, self.od)
self.assertIn(index, self.od)

for index in self.od:
if index < 0x0008:
# ignore dummies
continue
self.assertIn(self.od[index].name, exported_od)
self.assertIn(index, exported_od)

actual_object = exported_od[index]
expected_object = self.od[index]
self.assertEqual(type(actual_object), type(expected_object))
self.assertEqual(actual_object.name, expected_object.name)

if isinstance(actual_object, canopen.objectdictionary.ODVariable):
expected_vars = [expected_object]
actual_vars = [actual_object]
else:
expected_vars = [expected_object[idx] for idx in expected_object]
actual_vars = [actual_object[idx] for idx in actual_object]

for prop in [
"allowed_baudrates",
"vendor_name",
"vendor_number",
"product_name",
"product_number",
"revision_number",
"order_code",
"simple_boot_up_master",
"simple_boot_up_slave",
"granularity",
"dynamic_channels_supported",
"group_messaging",
"nr_of_RXPDO",
"nr_of_TXPDO",
"LSS_supported",
]:
self.assertEqual(getattr(self.od.device_information, prop),
getattr(exported_od.device_information, prop),
f"prop {prop!r} mismatch on DeviceInfo")

for evar, avar in zip(expected_vars, actual_vars):
self.assertEqual(getattr(avar, "data_type", None), getattr(evar, "data_type", None),
f" mismatch on {pretty_index(evar.index, evar.subindex)}")
self.assertEqual(getattr(avar, "default_raw", None), getattr(evar, "default_raw", None),
f" mismatch on {pretty_index(evar.index, evar.subindex)}")
self.assertEqual(getattr(avar, "min", None), getattr(evar, "min", None),
f" mismatch on {pretty_index(evar.index, evar.subindex)}")
self.assertEqual(getattr(avar, "max", None), getattr(evar, "max", None),
f" mismatch on {pretty_index(evar.index, evar.subindex)}")
if doctype == "dcf":
self.assertEqual(getattr(avar, "value", None), getattr(evar, "value", None),
f" mismatch on {pretty_index(evar.index, evar.subindex)}")

self.assertEqual(self.od.comments, exported_od.comments)