adafruit / Adafruit_CircuitPython_CircuitPlayground

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

added exception on importing lib on a non-cp board #124

Closed ChrisPappalardo closed 1 year ago

ChrisPappalardo commented 1 year ago

Added an import exception if the platform is not recognized when importing the lib

tekktrik commented 1 year ago

@caternuson, @Neradoc - we discussed this in the related issue (#121). What are y'all's thoughts?

caternuson commented 1 year ago

LGTM. Not sure what is messing with the doc build in the first CI run.

I'd suggest this approach vs. #125. If this is going to require defining a new class as in #125, then not worth the effort. This is a rare edge case and a minor point of confusion when hit.

ChrisPappalardo commented 1 year ago

Thanks for the feedback. The problem with this solution is that it breaks the doc build:

2023-04-24T22:07:12.0376311Z Warning, treated as error: 2023-04-24T22:07:12.0377270Z autodoc: failed to import module 'circuit_playground_base' from module 'adafruit_circuitplayground'; the following exception was raised: 2023-04-24T22:07:12.0377750Z linux not supported by this lib

That last line is the new code in this PR throwing an exception when the doc environment (linux) tries to import the lib.

125 is a workaround that throws the exception when the imported lib is used instead of on import. Can you help me understand why defining a new class is not worth the effort? It's a thin and generic object with just a few lines of code.

caternuson commented 1 year ago

Just a trade off. Having a new class in the code simply to be able to say "board not supported" could add other confusion and issues later for anyone wondering why it's there and what it's doing. The issue this is trying to solve is also hopefully rare. And when it does come up, is easily solvable with a few questions and discussion.

kattni commented 1 year ago

Closing in favor of #125

kattni commented 1 year ago

Reopening. I see that @caternuson prefers this over #125.

@caternuson The CI issue is failing because Sphinx is trying to run it from its own platform, and because Linux isn't supported, it fails. I tried adding the library itself to autodoc_mock_imports, and it fails as a warning on "Mock object detected." I found that you can suppress_warnings and this warning has a subtype of "mocked_object" or something close to that, but I tried a bunch of formats for adding that to conf.py, and none of them worked. There's probably something there, but I'm not seeing it.

caternuson commented 1 year ago

I don't think the original issue has ever even come up again. I'd be OK with just closing that issue and doing nothing. Esp. if it's going to take hacking shenanigans to get it to pass CI. I was originally hoping/thinking it could just a one-liner added to the existing if/else block.

kattni commented 1 year ago

@ChrisPappalardo Thanks so much for looking into this!

Thanks @caternuson. Closing this and the issue.

caternuson commented 1 year ago

Yep, thanks @ChrisPappalardo for giving it a go. Bummer that the CI/sphinx threw a bit of a wrench in this in terms of a solution.

ChrisPappalardo commented 1 year ago

@caternuson no worries, happy to help, glad we were able to close out the issue!