adafruit / Adafruit_CircuitPython_CircuitPlayground

CircuitPython library for Circuit Playground Express
MIT License
91 stars 71 forks source link

Better protect against using lib on wrong board? #121

Closed caternuson closed 1 year ago

caternuson commented 2 years ago

If someone attempts to import this lib:

from adafruit_circuitplayground import cp

on a non-Playground board, it'll throw an import error:

 ImportError: no module named 'adafruit_circuitplayground.cp'

which is potentially confusing.

Suggest updating the conditional: https://github.com/adafruit/Adafruit_CircuitPython_CircuitPlayground/blob/47f848f13f75d2f62d16407edaaf6dd0ec1fc3cc/adafruit_circuitplayground/__init__.py#L9-L12 to add a final else clause at the end to throw a more meaningful exception and message.

tekktrik commented 2 years ago

I think this is a neat idea; out of curiosity, how common is that issue?

caternuson commented 2 years ago

Not common at all. So far at least. It's similar to the using-wrong-neopixel-library issue. It can happen when following links form guides that link to guides that link to guides, etc. The original context was non-Playgorund but at some point can switch to being Playground specific. And code is just being copy pasted.

Thinking message would be something like "board not supported. this lib is for circuit playground boards only"

tekktrik commented 2 years ago

Makes sense to me!

Neradoc commented 2 years ago

Note that any new string would be added to the CPX frozen library build, where we fight for each byte we can get. It might be reasonably small though. Also these tests accept any NRF52 or samd21 anyway ? Is that ok ? Should we instead test board_id ? (if there is no backwards compatibility issue).

tekktrik commented 2 years ago

This change would be a good use case and test of the CI for checking memory that @FoamyGuy is working on, actually.

tekktrik commented 1 year ago

Is this still something we want to do? Just looking through old notifications that were not merged/closed.

ChrisPappalardo commented 1 year ago

working on this issue

kattni commented 1 year ago

After discussion, we've decided to close this issue. We have a couple of options for how to do it, but CI fails on one, and the second adds too much to the library. @caternuson said this issue has not come up since it was filed.