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

Purge workaround for docs build #509

Closed
erlend-aasland opened this issue Jul 9, 2024 · 6 comments · Fixed by #510
Closed

Purge workaround for docs build #509

erlend-aasland opened this issue Jul 9, 2024 · 6 comments · Fixed by #510

Comments

@erlend-aasland
Copy link
Contributor

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

try:
import can
from can import Listener
from can import CanError
except ImportError:
# Do not fail if python-can is not installed
can = None
CanError = Exception
class Listener:
""" Dummy listener """

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.

erlend-aasland added a commit to erlend-aasland/canopen that referenced this issue Jul 9, 2024
Always require dependencies to be installed.

Resolved christiansandberg#509
@friederschueler
Copy link
Collaborator

Yes, I think this is a good change.

@sveinse
Copy link
Collaborator

sveinse commented Jul 10, 2024

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
Copy link
Collaborator

acolomb commented Jul 10, 2024

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

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 d2748f9 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
Copy link
Collaborator

acolomb commented Aug 6, 2024

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

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

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.

4 participants