search

adafruit / Adafruit_CircuitPython_BH1750

CircuitPython library for use with the Adafruit BH1750 Ambient Light Sensor Breakout
MIT License
5 stars 3 forks source link

Change i2c bus parameter name to match most other libraries #6

Closed DeadSix27 closed 2 years ago

DeadSix27 commented 2 years ago

In most libraries offered by CircuitPython the i2c bus parameter name is simply i2c, in this library it isn't.

So, change it to i2c to match others and keep it unified.

Notable examples: https://github.com/adafruit/Adafruit_CircuitPython_ADS1x15/blob/main/adafruit_ads1x15/ads1x15.py#L69= https://github.com/adafruit/Adafruit_CircuitPython_BME680/blob/main/adafruit_bme680.py#L454= https://github.com/adafruit/Adafruit_CircuitPython_TCA9548A/blob/main/adafruit_tca9548a.py#L90=

FoamyGuy commented 2 years ago

Technically this would be a breaking change for user code that explicitly uses the argument name rather than relying on position.

I searched the Learn Guide repo and found only a single usage of this library and i2c is passed without the kwarg name so it should be unaffected.

dhalbert commented 2 years ago

This could be a major version change, to 2.0.0, since it's an API change. (I know you already released it.)

DeadSix27 commented 2 years ago

Technically this would be a breaking change for user code that explicitly uses the argument name rather than relying on

Agreed, was going to mention that too.

Looks good to me.

Thanks

FoamyGuy commented 2 years ago

Drat, I initially considered 2.0.0 but ended up changing to 1.1.0 when I made the release. Should have stuck with the first instinct.

If I recall correctly making a second release without any further changes to the library causes some trouble with the pip release. Should we try and see if maybe I'm misremembering?

Or try to find some small innocuous change and make a new PR from it and then a 2.x release?

DeadSix27 commented 2 years ago

Probably better to just resolve the missing typing (https://github.com/adafruit/Adafruit_CircuitPython_BH1750/issues/3) and then simply do a 2.0 release, until then, does it really matter whether its a minor or a major version change?

dhalbert commented 2 years ago

Probably no one will be affected by this anyway, so the 2.0.0 can be deferred to the next release.