Repository: christiansandberg/canopen

Issue:
Consider using ObjectDictionaryError iso. NotImplementedError in export/import OD APIs
Discovered while reviewing #469. (See https://github.com/christiansandberg/canopen/pull/469#discussion_r1661613953.)

In most cases, `ObjectDictionaryError` is used for the `export_od` and `import_od` APIs, but for unsupported file formats, a `NotImplementedError` is raised. This is unconventional; `NotImplementedError` is not a good fit for such an error case, as it was designed for other purposes (see the CPython docs).

The `NotImplementedError` in `export_od` is fairly new (introduced with #469), and has not been a part of a formal release; it can be changed without backwards compatibility concerns. The `raise` in `import_od` has been there longer, _but_ these are (for now) undocumented, though public, APIs. One can argue that since they are undocumented, it may be ok to change `import_od` as well.

Consider switching to `ObjectDictionaryError`.

Discussion:
codecov-commenter: :warning: Please install the !['codecov app svg image'](https://github.com/codecov/engineering-team/assets/152432831/e90313f4-9d3a-4b63-8b54-cfe14e7ec20d) to ensure uploads and comments are reliably processed by Codecov.

## [Codecov](https://app.codecov.io/gh/christiansandberg/canopen/pull/476?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Christian+Sandberg) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 67.60%. Comparing base [(`5db5913`)](https://app.codecov.io/gh/christiansandberg/canopen/commit/5db59130423948e1e8034a67983ca00afd0269d8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Christian+Sandberg) to head [(`e5fcd1d`)](https://app.codecov.io/gh/christiansandberg/canopen/commit/e5fcd1d2385d8e4403d3bedd3f83314e399d7863?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Christian+Sandberg).

:exclamation: Your organization needs to install the [Codecov GitHub app](https://github.com/apps/codecov/installations/select_target) to enable full functionality.

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/christiansandberg/canopen/pull/476/graphs/tree.svg?width=650&height=150&src=pr&token=3wUNWZVohh&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Christian+Sandberg)](https://app.codecov.io/gh/christiansandberg/canopen/pull/476?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Christian+Sandberg)

```diff
@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
+ Coverage   67.52%   67.60%   +0.07%     
==========================================
  Files          26       26              
  Lines        3098     3099       +1     
  Branches      521      521              
==========================================
+ Hits         2092     2095       +3     
+ Misses        864      862       -2     
  Partials      142      142              
```

| [Files](https://app.codecov.io/gh/christiansandberg/canopen/pull/476?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Christian+Sandberg) | Coverage Δ | |
|---|---|---|
| [canopen/objectdictionary/\_\_init\_\_.py](https://app.codecov.io/gh/christiansandberg/canopen/pull/476?src=pr&el=tree&filepath=canopen%2Fobjectdictionary%2F__init__.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Christian+Sandberg#diff-Y2Fub3Blbi9vYmplY3RkaWN0aW9uYXJ5L19faW5pdF9fLnB5) | `81.84% <100.00%> (+0.72%)` | :arrow_up: |

</details>
erlend-aasland: I'm fine with `ValueError`; it is a good alternative to `ObjectDictionaryError`.

Patch:
diff --git a/canopen/objectdictionary/__init__.py b/canopen/objectdictionary/__init__.py
index d9a2a2f..441ade9 100644
--- a/canopen/objectdictionary/__init__.py
+++ b/canopen/objectdictionary/__init__.py
@@ -50,7 +50,11 @@ def export_od(od, dest: Union[str, TextIO, None] = None, doc_type: Optional[str]
             from canopen.objectdictionary import eds
             return eds.export_dcf(od, dest)
         else:
-            raise NotImplementedError(f"No support for the {doc_type!r} format")
+            allowed = ", ".join(doctypes)
+            raise ValueError(
+                f"Cannot export to the {doc_type!r} format; "
+                f"supported formats: {allowed}"
+            )
     finally:
         # If dest is opened in this fn, it should be closed
         if opened_here:
@@ -88,7 +92,12 @@ def import_od(
         from canopen.objectdictionary import epf
         return epf.import_epf(source)
     else:
-        raise NotImplementedError("No support for this format")
+        doc_type = suffix[1:]
+        allowed = ", ".join(["eds", "dcf", "epf"])
+        raise ValueError(
+            f"Cannot import from the {doc_type!r} format; "
+            f"supported formats: {allowed}"
+        )
 
 
 class ObjectDictionary(MutableMapping):


Test patch:
diff --git a/test/test_eds.py b/test/test_eds.py
index e023b3b..2275e48 100644
--- a/test/test_eds.py
+++ b/test/test_eds.py
@@ -54,6 +54,10 @@ class TestEDS(unittest.TestCase):
         with self.assertRaises(IOError):
             canopen.import_od('/path/to/wrong_file.eds')
 
+    def test_load_unsupported_format(self):
+        with self.assertRaisesRegex(ValueError, "'py'"):
+            canopen.import_od(__file__)
+
     def test_load_file_object(self):
         with open(EDS_PATH) as fp:
             od = canopen.import_od(fp)
@@ -198,7 +202,7 @@ class TestEDS(unittest.TestCase):
                 self.verify_od(tempfile, doctype)
 
             # Test for unknown doctype
-            with self.assertRaises(NotImplementedError):
+            with self.assertRaisesRegex(ValueError, "'unknown'"):
                 tempfile = str(Path(tempdir, "test.unknown"))
                 canopen.export_od(self.od, tempfile, doc_type="unknown")