search

adafruit / Adafruit_CircuitPython_BME280

CircuitPython driver for the BME280
MIT License
63 stars 42 forks source link

Code fails duplicate-code check, consider refactor #61

Closed evaherrada closed 1 year ago

evaherrada commented 2 years ago

https://github.com/adafruit/Adafruit_CircuitPython_BME280/runs/6993430685?check_suite_focus=true#step:11:33

kattni commented 2 years ago

Assigning Tekktrik as well to look into this. We'll work together to decide what the best solution is.

tekktrik commented 2 years ago

Interesting that it thinks it's duplicate code even though one is I2CDevice and the other is SPIDevice. This could also be refactored, but see more of an argument for ignoring it given it's not truly duplicate.

tekktrik commented 2 years ago

Oh, never mind, I read the CI wrong. It's likely we could set up a base class for each device type.

tekktrik commented 2 years ago

This library is a candidate for refactoring based on pylint warning of duplicate code. The solution for this library would be to refactor the code to not have duplicated portions of code. In most cases, this might mean making base classes/functions so code can be recycled, but in some cases it may mean removing duplicate functionality (e.g., for compatibility).

To determine the duplicate code section, allow pylint to warn against duplicated code by removing ,duplicate-code from .pre-commit-config.yaml and then running pre-commit:

https://github.com/adafruit/Adafruit_CircuitPython_BME280/blob/816def6ec432f83b107e74f4002f53cd712c0fc9/.pre-commit-config.yaml#L27

Then, refactor the duplicated code sections until it passes pre-commit. The modified .pre-commit-config.yaml file and refactored code can then be submitted as a PR.

rsbohn commented 2 years ago

Going to give the refactor a try. #CircuitPythonDay2022

rsbohn commented 2 years ago

I've got a pull request: https://github.com/adafruit/Adafruit_CircuitPython_BME280/pull/62

Questions about naming things:

I extracted the bus specific parts to classes named I2C_Impl and SPI_Impl in protocol.py. Are these *_Impls, proxies, or delegates? Is there a better name than protocol.py?

rsbohn commented 2 years ago

Tested with Blinka/MCP2221. Tested with 8.0.0-beta.0 on 2022-08-18; Adafruit QT Py M0 with samd21e18: Had to run the .mpy; no room for the advanced.py or .mpy files. But it worked!

from adafruit_bme280 import basic

i2c = board.I2C()
sensor = basic.Adafruit_BME280_I2C(i2c, address=0x76)
print("Temp:",sensor.temperature)
print("RH:  ",sensor.relative_humidity)
print("Alt: ",sensor.altitude)