search

adafruit / Adafruit_CircuitPython_ESP32SPI

ESP32 as wifi with SPI interface
MIT License
103 stars 75 forks source link

__init__ arguments are not Pins #121

Closed dhalbert closed 2 years ago

dhalbert commented 3 years ago

The arg names here are suffixed as _pin, but they should actually be DigitalInOut objects. The arg names could be clearer, maybe _dio or just drop the suffix. At least one user might have been confused by this: https://forums.adafruit.com/viewtopic.php?f=60&t=173353.

https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/blob/fce466bd2bb70ca86b79e5cb36bbaca00afacfd1/adafruit_esp32spi/adafruit_esp32spi.py#L136-L138

brentru commented 3 years ago

@dhalbert I think dropping the suffix would be clearer compared to substituting _pin for _dio

tekktrik commented 3 years ago

Is this something we want to do? I'm happy to submit a PR for this :) Was also planning to add type annotations, if that helps at all.

FoamyGuy commented 2 years ago

resolved by #146

FoamyGuy commented 2 years ago

@tekktrik all of the other instances of similar *_pin named arguments are linked above here in this issue now.

These ones do have Learn Guide code using the named keyword arguments as well, so we will need to make sure to update all usages at the same time as the library changes are made.

tannewt commented 2 years ago

I agree with @brentru that dropping the suffix is clearer than changing it to _dio. Adding the suffix is redundant with the type info.

tekktrik commented 2 years ago

I can submit another PR to drop it if that's the case

tannewt commented 2 years ago

I think it's important to point out that by taking DigitalInOut its usually meant to work with GPIO expander provided objects as well. Pin should be only used when a native pin object is expected.