christiansandberg / canopen

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

Purge workaround for docs build #509

Closed erlend-aasland closed 2 months ago

erlend-aasland commented 2 months ago

canopen/network.py contains the following docs build workaround:

https://github.com/christiansandberg/canopen/blob/3aa509db9fea8094516e5eb717be3df182f0751a/canopen/network.py#L8-L17

I suggest to purge this hack from network.py; the RTD config already installs canopen as a package, which implicitly installs can. Moreover, purging this workaround will make it easier to add type annotations for network.py.

friederschueler commented 2 months ago

Yes, I think this is a good change.

sveinse commented 2 months ago

Yes, please. With type annotation checks we'd need code something like this to circumvent the conditional:

from typing import TYPE_CHECKING

try:
    import can
    from can import Listener
    from can import CanError
except ImportError:
    # Type checkers don't like this conditional logic, so it is only run when
    # not type checking
    if not TYPE_CHECKING:
        # Do not fail if python-can is not installed
        can = None
        CanError = Exception
        class Listener:
            """ Dummy listener """
acolomb commented 2 months ago

I'd like to go back to the basics of this issue. How exactly is this related to the documentation build?

Could it have merit for other use cases?

Sorry, I just haven't fully grasped what this passage is meant to do in the first place.

erlend-aasland commented 2 months ago

I'd like to go back to the basics of this issue. How exactly is this related to the documentation build?

The except clause was historically added and changed with these commits (new to old):

When these workarounds were added, there was no doc/requirements.txt, no .readthedocs.yaml. Not having python-can installed would render the library totally useless at any of these three points in time; hence, the changes were made because of other reasons, and d2748f958d3b8d47cc995c7fbab7510e6bafe7f6 is explicit about it: fix the RTD build.

Could it have merit for other use cases?

No; the code is completely broken if the imports fail; python-can is a hard dependency for canopen. This goes for the state of the code base for all three referenced commits.

acolomb commented 1 month ago

Ah, thanks for the explanation @erlend-aasland. That's what I wanted to know, and especially it seems there was no special funky use-case that required this.

erlend-aasland commented 1 month ago

No sweat, it's always good to be reminded about Chesterton's Fence :)