od: Robust path handling without type errors

[acolomb]

Fix the union-attr errors squelched in #651 properly. In the process, extend import_od() to gracefully handle some edge cases around missing file names and non-str paths in the source argument.

Extend unit tests around import and export functions. Note that these are still a bit convoluted because the EDS import is tested through the generic, format-agnostic OD API. This is still much easier than splitting tests between generic and format-specific though. If ever someone adds unit tests covering the EPF import, that might need some refactoring.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[bizfsc]

A few suggestions below.

canopen/objectdictionary/init.py

the standard library already has the right abstraction for this. contextlib.nullcontext wraps a caller-owned object in a no-op context manager, so the try/finally and the flag variable can be replaced with a plain with statement:

    import contextlib

    if isinstance(dest, (str, os.PathLike)):
        if doc_type is None:
            _, suffix = os.path.splitext(os.fspath(dest).lower())
            for t in supported_doctypes:
                if suffix == f".{t}":
                    doc_type = t
                    break
            else:
                doc_type = "eds"
        # A path was given: open the file here. The with statement
        # guarantees it is closed on exit, including on exceptions.
        ctx = open(dest, 'w')
    else:
        # A file-like object or None (stdout) was passed in. The caller owns
        # its lifecycle; use a no-op context manager so the with block below
        # works uniformly without closing the caller's object.
        ctx = contextlib.nullcontext(dest)

    with ctx as dest:
        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)

test/test_eds.py

test_export_eds_auto_close has three issues:

First, canopen.export_od(self.od, m_open) passes a callable Mock as dest. This works by accident because Mock satisfies any hasattr check, but it does not actually test the intended behaviour.

Second, fd = m_open() calls the mock again, adding an unwanted call to its call_count. Use m_open.return_value instead.

Third, with the contextlib approach above the with statement calls exit, not close() directly. The assertion should match:

    def test_export_eds_auto_close(self):
        # File-like object passed in directly: must NOT be closed by export_od
        with io.StringIO() as dest:
            canopen.export_od(self.od, dest, "eds")
            self.assertFalse(dest.closed)
        for path in ("mock.eds", pathlib.Path("mock.eds")):
            with self.subTest(path=path):
                m_open = mock_open()
                with patch("canopen.objectdictionary.open", m_open):
                    canopen.export_od(self.od, path)
                # File object opened at path must be closed before return
                m_open.return_value.__exit__.assert_called_once()

    def test_export_eds_auto_close_exception(self):
        m_open = mock_open()
        m_open.return_value.write.side_effect = IOError("Simulated write failure")
        with patch("canopen.objectdictionary.open", m_open), self.assertRaises(IOError):
            canopen.export_od(self.od, "mock.eds")
        # File object opened at path must be closed even when an exception is raised
        m_open.return_value.__exit__.assert_called_once()

Note: in the original test_export_eds_auto_close_exception the assert_called_once() call is inside the assertRaises block, which means it is unreachable when no exception occurs and silently does nothing when one is raised. It must be outside.

test_load_unsupported_format: remove the canopen.import_od(object()) case. object() is outside the declared signature and the behaviour (returning "" from getattr fallback) is an implementation detail, not a contract. If a caller passes a wrong type, that is their problem.

Finally, add a positive test for pathlib.Path in import_od. The PR only adds a not-found error case. Add:

    def test_load_pathlib_path(self):
        od = canopen.import_od(pathlib.Path(SAMPLE_EDS))
        self.assertTrue(len(od) > 0)