Skip to content
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.

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

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

Closed
erlend-aasland opened this issue Jul 3, 2024 · 4 comments · Fixed by #493
Closed

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

erlend-aasland opened this issue Jul 3, 2024 · 4 comments · Fixed by #493

Comments

@erlend-aasland
Copy link
Contributor

PdoBase.export() depends on the third-party module canmatrix, which was removed from the canopen requirements with commit c46228f, 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 added a commit to erlend-aasland/canopen that referenced this issue Jul 3, 2024
The canmatrix dependency was removed on Oct 10, 2021
with commit c46228f.

Resolves christiansandberg#488
erlend-aasland added a commit to erlend-aasland/canopen that referenced this issue Jul 3, 2024
The canmatrix dependency was removed on Oct 10, 2021
with commit c46228f.

Resolves christiansandberg#488
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jul 3, 2024

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

  1. Fix: erlend-aasland@69444db
  2. Purge: erlend-aasland@976c6d7

erlend-aasland added a commit to erlend-aasland/canopen that referenced this issue Jul 3, 2024
The canmatrix dependency was removed on Oct 10, 2021
with commit c46228f.

Resolves christiansandberg#488
@erlend-aasland
Copy link
Contributor Author

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 erlend-aasland@69444db.

@acolomb
Copy link
Collaborator

acolomb commented Jul 3, 2024

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
Copy link
Contributor Author

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

erlend-aasland added a commit to erlend-aasland/canopen that referenced this issue Jul 3, 2024
The canmatrix optional dependency was removed on Oct 10, 2021 with commit
c46228f. It is now added back as an optional dependency, using the same
name as previously: db_export.

To install the dependency:

$ python3 -m pip install 'canopen[db_export]'

Resolves christiansandberg#488
erlend-aasland added a commit to erlend-aasland/canopen that referenced this issue Jul 3, 2024
The canmatrix optional dependency was removed on Oct 10, 2021 with commit
c46228f. It is now added back as an optional dependency, using the same
name as previously: db_export.

To install the dependency:

$ python3 -m pip install 'canopen[db_export]'

Resolves christiansandberg#488
@acolomb acolomb closed this as completed in 6bd39ba Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants