-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
docs/library/neopixel.rst: Update neopixel docs to hide dunders. #16764
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: tobes <toby.junk@gmail.com>
@@ -54,17 +54,17 @@ Pixel access methods | |||
Sets the value of all pixels to the specified *pixel* value (i.e. an | |||
RGB/RGBW tuple). | |||
|
|||
.. method:: NeoPixel.__len__() | |||
.. method:: len(np) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no method Neopixel.len
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise it's not a method but the .. method::
directive is as much about formatting as anything else. What would you suggest. As I say I was using pythons documentation as a guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend to leave the docs unchanged - as changing them will likely break the hyperlinks in the documentation , and surely break the generation of the micropython-stubs, which actually parse the directives.
Please see my comment for recommendations
|
||
.. method:: NeoPixel.__setitem__(index, val) | ||
.. method:: np[index] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no method Neopixel.np
The definition and documentation of the dunder methods should not be removed. |
I'm not aware of dunder methods being documented as 'real' methods in any python documentation. They aren't public methods as such because of how you access them. To me it is like saying Part of the python philosophy is to respect private methods and imho the current documentation goes against that. |
Ok, but there seem to be very few dunder methods and mostly around email so it still feels fairly niche. Eg very few I don't know enough about the NeoPixel code but it still feels odd that the natural way to use an instance isn't documented apart from one line in the code example, which might confuse some users as there is no explanation. I still feel that these ways to access the object should be documented for users less familiar with python. |
Summary
Change documentation to show using
len(np)
to access number of pixels rather thanNeoPixel.__len__()
. Also use update for setting and getting items vianp[index]
.I was looking at the NeoPixel documentation and saw that it was showing dunder methods for accessing number of pixels and also for setting/reading pixels ie
NeoPixel.__len__()
,NeoPixel.__setitem__()
,NeoPixel.__getitem__()
rather than usinglen(np)
,np[index]
.It is good practice especially for beginners that we should be showing correct access of these items rather than encouraging people to use private methods.
I used the python documentation https://docs.python.org/3/library/stdtypes.html#mapping-types-dict as a basis for this update.
I chose
np
as a place holder for a NeoPixel instance as it felt appropriate, additionally I updated the code example to usenp
instead ifn
so that there was consistency accross the documentation.I think the Methods sections should also be combined but I have not included that change here but can if you think that is the way to go.
Testing
This is a trivial documentation change so no testing was done