christiansandberg / canopen

CANopen for Python
http://canopen.readthedocs.io/
MIT License
437 stars 195 forks source link

`PdoBase.export()` is broken bco. broken dependencies #488

Closed erlend-aasland closed 2 months ago

erlend-aasland commented 3 months ago

PdoBase.export() depends on the third-party module canmatrix, which was removed from the canopen requirements with commit c46228f9cf2d2661166d68e7175f3e8b99064194, three years ago. This implies it has been de facto broken since that commit, unless the user accidentally had a compatible canmatrix library installed.

There's a couple of ways to mitigate this:

  1. add back the canmatrix dependency, fix .export() (if it needs fixing), and add unit tests in order to prevent future regressions like this
  2. acknowledge that the API is de facto broken and purge it from canopen

FWIW, my personal preference would be 2).

Discovered while working on increasing PDO coverage to 100%.

erlend-aasland commented 3 months ago

I've added PoC-branches for each proposed solution. Happy to open a PR for whatever variant you prefer.

  1. Fix: https://github.com/erlend-aasland/canopen/commit/69444dbf0db231dc861410960f4a1803ce783564
  2. Purge: https://github.com/erlend-aasland/canopen/commit/976c6d75aa88b34e69d18896788a11c1cdd04a00
erlend-aasland commented 3 months ago

There's also a third way to mitigate this: conditionally add PdoBase.export, depending on if canmatrix is installed or not. This can be trivially added on top of https://github.com/erlend-aasland/canopen/commit/69444dbf0db231dc861410960f4a1803ce783564.

acolomb commented 3 months ago

I prefer the third solution. It seems this is the only place where the dependency is used, so forcing all users to install it is too much. But the mapping export function itself is surely useful.

Can we make sure that the canmatrix package is installed when running tests? Like adding a "test-only dependency" to the pytest config?

Ideally, the function would just raise a more helpful exception in case the import fails.

erlend-aasland commented 3 months ago

Yep, the third solution is probably easiest on users. I'll create a PR shortly.