Consider using ObjectDictionaryError iso. NotImplementedError in export/import OD APIs

[erlend-aasland]

Discovered while reviewing #469. (See #469 (comment).)

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.

[erlend-aasland]

For context, quoting the relevant CPython docs:

This exception is derived from RuntimeError. In user defined base classes, abstract methods should raise this exception when they require derived classes to override the method, or while the class is being developed to indicate that the real implementation still needs to be added.

[sveinse]

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.

That is incorrect, #469 only moved the code around. See

canopen/canopen/objectdictionary/__init__.py

Line 88 in 1fc63b5

raise NotImplementedError("No support for this format")

edit. This was the code that introduced it, 8 year ago: 7fdd90c

[erlend-aasland]

PTAL. 7fdd90c introduced NotImplementedError to import_od; 5db5913 introduced NotImplementedError to export_od.

[erlend-aasland]

Moreover, #469 did absolutely not just move code around; it was created for a specific behavioural change (closing the file), and it accidentally (?) introduced two behavioural changes.

[sveinse]

Yeah, you're right. I guess I simply copied the same logic has the other function to make them consistent and issue the same type exception as the other function, albeit of incorrect type.

[erlend-aasland]

Well, IMO, these should be changed to ObjectDictionaryError before documenting them (see #473). Then we can also add :raises: ObjectDictionaryError directives to the docstrings in #473. If these is an agreement about this, I'll be happy to propose a PR.

[sveinse]

I support this. Not sure about ObjectDictionaryError thou. Technically its not an error with the OD, its an unknown file suffix. Other that than I don't have any strong opinions about which exception type is used here.

[erlend-aasland]

Related, regarding the other uses of NotImplementedError:

• canopen/sdo/base.py: these look fine to me; PdoBase is a base class, and upload/download are de facto abstract methods to be implemented by subclasses.
• canopen/variable.py: ISTM the same can be said about the Variable base class. Though, their error messages are a little bit confusing; IMO, the error messages should help the user (programmer) to understand how to mitigate the problem, but this is out of scope for this issue.

[erlend-aasland]

I support this. Not sure about ObjectDictionaryError thou. Technically its not an error with the OD, its an unknown file suffix. Other that than I don't have any strong opinions about which exception type is used here.

One solution could be to introduce a new exception type, subclasses off of ObjectDictionaryError, for example ObjectDictionaryFormatError, but I'm not sure that's needed.

Another candidate could be ValueError. The CPython docs say:

Raised when an operation or function receives an argument that has the right type but an inappropriate value, and the situation is not described by a more precise exception such as IndexError.

[acolomb]

How about just using a generic RuntimeError for unsupported file formats?

I don't think it really matters much which exception we raise here, in terms of keeping compatibility. Because when a wrong file type is supplied, there is not much an application can "work around" to fix it. Catching specific exceptions is, in my experience, useful if you have an idea of what can be done to remedy a specific, expected failure. But what would that ever look like if the supplied file is in the wrong format? Silently retry with a different file? I don't think so. Such a failure is thus an "exceptional" condition, worthy of being simply thrown back at whoever supplied the file. If it was the user, then a file-picker was wrongly configured or a command line argument should have been better validated before. If it was programatically from application code, then that needs fixing by the developer, who doesn't care what exception type was printed on the console, but just checks the error message.

[acolomb]

Another candidate could be ValueError. The CPython docs say:

Surely there is room for improvement here in general. Please point out specific locations in a PR, so we can discuss them individually.

EDIT: Sorry, I misunderstood that you're suggesting a ValueError exception to be used here for the wrong format case. That's perfectly valid. I took your comment as ValueError being another candidate for switching to more specific exceptions in other places.

[erlend-aasland]

Because when a wrong file type is supplied, there is not much an application can "work around" to fix it.

The same can be said about some of the other ObjectDictionaryErrors¹. A valid use case could be a GUI app that let the user load a "config" file, and then using for example import_od to validate it. If import_od raises ObjectDictionaryError, display an error dialog to inform the user about the offending file. For a console app where you have a config file that let's the user specify which EDS file to load, I would validate the file and exit gracefully if import_od failed.

IOW, I think there is value in normalising the exceptions raised when importing (and possibly also exporting) ODs.

I can create a draft PR so it is easier to discuss the proposed changes.

Footnotes

1. Mismatch between expected and actual data size ↩