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

Improve exception raising #112

Open
RobertoRoos opened this issue Feb 15, 2021 · 2 comments
Open

Improve exception raising #112

RobertoRoos opened this issue Feb 15, 2021 · 2 comments

Comments

@RobertoRoos
Copy link
Contributor

This followed the discussion in #108 : it would be nice if the exceptions raised by hidapi could be more easily distinguished.

This goes in two ways:

  • They should be more understandable to a human seeing the uncaught exception (e.g. "Opening the HID device failed" instead of "open failed", there seems no reason to be brief in the error message)
  • They should be easier to catch (e.g. try ... except hid.ConnectionError, instead of having to rely on the error message)

There was the discussion in the link above that renaming the exact "open failed" message would break compatibility. Do other people think this is true?

I personally it is worth taking the risk if it improves the package. If people cannot upgrade they can rely on their package manager.

@bearsh
Copy link
Contributor

bearsh commented Feb 15, 2021

I agree with you. maybe it's worth to collect such api breaking improvements and release them together in major version bump...

@jonasmalacofilho
Copy link
Contributor

jonasmalacofilho commented Feb 15, 2021

I'll summarize my arguments here since they are now hidden by default (by GitHub) in #108.

IOError('open failed') is a particularly useful exception to catch, and unfortunately relying on the exact message is currently the only way to differentiate between this specific failure case (commonly caused by insufficient permissions) and other OSError exceptions (also note that IOError is just an alias to OSError) happening in the system.

(One could catch the 'open failed' exception right where the call to open/open_path is made; but the whole point of exceptions is to handle errors closer to where something useful can be done about them).

Replacing this (and in other specific failures cases) with custom exceptions is certainly an improvement. However, the issue of backward compatibility arises because relying on changes to the version number has significant limitations. First, only last November did pip finally started to properly enforce version requirements, and it may take some time for the new behavior to be widely used.

More importantly, dependent programs that are packaged in Linux Distributions need to be compatible with the whatever version of cython-hidapi that the distribution has packaged. Stricter requirements may be honored, but could lead to the removal of the package if there are unsolvable conflicts.

(Incompatible versions of large dependencies (like Python itself) are usually handled with suffixes (e.g python39 vs python38). But I don't think it's reasonable to ask distributions to package cython-hidapi that way, and even they humored us, it would again take some time for this to be widely available).

Finally, even in perfect requirement enforcement scenarios, not all dependent projects will want to break compatibility with previous versions of cython-hidapi. Virtual environments are a solution for dependency conflicts, but they are not super friendly to end users, and some projects may simply prefer to deal with the pain themselves.

Because of all of these reasons I'm advocating for this pain to be made as minimal as possible and, preferably, for no changes that introduce known ways of breaking dependent packages:

  • make all new custom exceptions extend the previous standard ones (i.e. OSError)
  • keep the first argument to the exception for failure to open a device equal to 'open failed'

(Note that it is probably possible to have a better user message while keeping the old first argument. I could look into that if you want).


P.S. I took a quick look at GitHub's list of cython-hidapi dependent repositories, and liquidctl, which I maintain and am using as a basis for these arguments, is fairly high in the number-of-stars contest. While I plan to support both old and new behaviors (and want to keep the necessary shims as small as possible), I am also advocating for projects that may only notice breaking changes once bug reports start to come in. And there are also the couple of projects that depend on liquidctl and that could be exposed to cython-hidapi changes before they can update to a new version of liquidctl.

P.P.S. @bearsh, cython-hidapi versioning mirrors that of hidapi itself, so a major version bump would be out of character (even if it solved all compatibility issues).

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

No branches or pull requests

3 participants