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

Listener.stop() method breaks API since declared abstract #1770

Open
acolomb opened this issue Apr 24, 2024 · 2 comments
Open

Listener.stop() method breaks API since declared abstract #1770

acolomb opened this issue Apr 24, 2024 · 2 comments
Labels

Comments

@acolomb
Copy link
Contributor

acolomb commented Apr 24, 2024

In #1724, the can.listener.Listener.stop() method was decorated as an @abstractmethod, without any further explanation. This seems to work alright within the python-can codebase, but other dependent projects will get in trouble if they don't implement that method on derived classes. At the very least should this API change be documented prominently.

This actually causes trouble with e.g. https://github.com/christiansandberg/canopen/, see https://github.com/christiansandberg/canopen/blob/713f005af8a6787a4ff54a26f55fa6736f86abaf/canopen/network.py#L346

Expected behavior

This API change should be clearly documented OR reverted, going back to the empty default implementation.

Additional context

OS and version: Linux Ubuntu 22.04
Python version: 3.11.6
python-can version: v4.3.1-16-g7dba4490 (current main branch)
python-can interface/s (if applicable): does not matter

Example script to reproduce
import can

class MessageListener(can.Listener):

    def on_message_received(self, msg):
        pass

MessageListener()
# TypeError: Can't instantiate abstract class MessageListener with abstract method stop
@acolomb acolomb added the bug label Apr 24, 2024
@zariiii9003
Copy link
Collaborator

You are correct, it must be mentioned in the changelog #1759. Another reason not to release it as 4.3.2, but 4.4.0.
Thanks for testing the pre-release!

@acolomb
Copy link
Contributor Author

acolomb commented Apr 24, 2024

You're welcome. I actually only switched to the main branch for investigating a fix for #1771.

Will make a PR in the canopen package to deal with this change, to hopefully stay compatible right after your v4.4.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants