sde1000 / python-dali

Library for controlling DALI lighting systems
Other
150 stars 71 forks source link

Who is relying on the exceptions raised by dali.frame? #93

Closed sde1000 closed 2 years ago

sde1000 commented 2 years ago

At the moment, dali.frame.Frame raises exceptions in the following circumstances:

  1. If you try to create a Frame() and pass a sequence to initialise it, and one or more of the elements of the sequence is less than 0 or greater than 255, it will raise TypeError
  2. If you call pack_len() on an instance of Frame and the length you specify won't accommodate the frame (eg. passing length 2 for a 24-bit frame) it will raise ValueError
  3. (And other circumstances, but we're not discussing those here)

The first case isn't covered by the test suite; the second case is.

I'm looking at using int.from_bytes() and int.to_bytes() in the implementations of these, for a minor speedup. They've been available since Python 3.2. (The frame code was originally written for Python 2.7. Using the native methods gives about a 5% speedup running the tests on my machine.)

These native methods raise ValueError in the first case, and OverflowError in the second case.

The question is: is anybody relying on the current behaviour? I can catch the exceptions raised by the native methods and reproduce the current behaviour, but the choice of exceptions in the native methods does seem more appropriate.

rnixx commented 2 years ago

Personally i don't rely on these concrete exception types. In my opinion the behavioral change is fine.

If anyone concerns, it may be worth implementing a "legacy" mode mimic the current behavior.

BTW - the changelog is outdated. Pypi release is 0.7.1 while last documented changes are from 0.6. I state this because i think documentation is the crucial part about behavioral changes ;).

sde1000 commented 2 years ago

I'll make the change, keeping support for the existing behaviour by default and hiding the new exceptions behind a keyword argument. I'll add a DeprecationWarning about the existing behaviour.

I've brought the CHANGES.rst file up to date. :-)