adafruit / Adafruit_CircuitPython_NeoPixel

CircuitPython drivers for neopixels.
MIT License
308 stars 99 forks source link

should fill be a property? #16

Closed caternuson closed 7 years ago

caternuson commented 7 years ago

Just a suggestion. Discuss.... This:

pixels.fill = 0xFFBAD2

instead of this:

pixels.fill(0xFFBAD2)
ladyada commented 7 years ago

i think since it does it for each pixel, im leaning towards function...its not a property its an action "to fill" :)

caternuson commented 7 years ago

by "property", i was thinking a python @ property function, not just a class member variable

i'd expect the function to actually do the fill (set all pixels to color) as well

dhalbert commented 7 years ago

I agree with @ladyada. It's not a property, because it does something. It doesn't make sense to get the value (f = np.fill), only to set it.

You might make a similar argument that setting .brightness operates on all the pixels, but that preserves the states of the individual pixels: it turns a notional brightness knob on the whole set of pixels. And, fetching the brightness makes sense.

caternuson commented 7 years ago

sorry sorry. i meant an @ setter. (i use the term property too loosely)

@fill.setter
def fill(self, color):
        """Colors all pixels the given ***color***."""
        auto_write = self.auto_write
        self.auto_write = False
        for i in range(len(self)):
            self[i] = color
        if auto_write:
            self.show()
        self.auto_write = auto_write
tannewt commented 7 years ago

@caternuson I agree with @dhalbert and @ladyada.

Remember that its a matter of what it looks like from the outside. It doesn't matter that its a function internally. From the outside it should make sense as a variable rather than a function. Properties just make it convenient to store variables in a different way than they appear. For example, pixels is a bytearray rather than a list of tuples. Another example is the adc where the value is actually "stored" on the device.

caternuson commented 7 years ago

Thanks for the responses. I think that's enough to go ahead and close for now.