search

adafruit / Adafruit_CircuitPython_PM25

CircuitPython library for PM2.5 sensors
MIT License
28 stars 16 forks source link

Non necessary import of I2C if using an UART sensor #5

Closed dglaude closed 3 years ago

dglaude commented 4 years ago

When using the UART sensor, you don't need the adafruit_bus_device library.

However, the following line in the library import what is required for the I2C version: from adafruit_bus_device.i2c_device import I2CDevice

No big deal, and I have no clue how to do things better because all my idea seems to be worse that current situation (like putting the import on the I2C side or separating the two). Also I don't have the I2C version to test, so I could not really test a change on that side.

But it could be an improvement for user of UART sensor.

evaherrada commented 4 years ago

@dglaude I know that we have done something like that (putting the i2c import in the i2c class) a few times before and just put a pylint ignore on that line.

dglaude commented 4 years ago

I believe @kattni has the hardware to test this, so it is best that she test this option. I prefer not to blindly propose a change.

kattni commented 4 years ago

@dglaude You may be able to move from adafruit_bus_device.i2c_device import I2CDevice into the PM25_I2C class. https://github.com/adafruit/Adafruit_CircuitPython_PM25/blob/f70830bcf712e3f74c4756ed663b4fd17029c9a3/adafruit_pm25.py#L122 Then it would only be imported if using I2C. It may require a pylint: disable= for an import in the "wrong" place. I can test the changes if you'd like to put in a PR.

dglaude commented 4 years ago

I have been trying that in #7 but black is not happy yet and is not telling me exactly why. (and then I will have to keep pylint happy too) As I don't have a local pylint/black install, I think I will give up for now.

dhalbert commented 4 years ago

@dglaude if you have pip3, you can install black and pylint just with: pip3 install --user black pylint. You may not need the --user, and if you are on Windows, it may be pip instead of pip3.

dglaude commented 4 years ago

Thanks, I will read this guide: https://learn.adafruit.com/improve-your-code-with-pylint/overview I have already the right pylint exclude, now I need to figure out what black want from me.

dglaude commented 4 years ago

Should be solved with #7 . This require testing with I2C air quality sensor, but it works in UART and I2C without sensor.

dglaude commented 4 years ago

The Pull Request #8 might solve this a better way than #7 However, as a package, the way to import and use is a bit different. I am no expert in "import ... as ..." that could limit this issue.

Please consider the best option, also @kattni if some learn guide use this, you may want to sync. Finally @gadgetoid has plan to see if he can bring improvement from Pimoroni UART only library. He might want to do that after the merge.

dglaude commented 3 years ago

8 is now accepted, this solve the problem.