Skip to content

Fix up docs #1

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 20, 2018
Merged

Fix up docs #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,17 @@ Introduction
============

.. image:: https://readthedocs.org/projects/adafruit-circuitpython-mcp4725/badge/?version=latest

:target: https://circuitpython.readthedocs.io/projects/mcp4725/en/latest/

:alt: Documentation Status

.. image :: https://img.shields.io/discord/327254708534116352.svg
:target: https://discord.gg/nBQh6qu
:alt: Discord

.. image:: https://travis-ci.org/adafruit/Adafruit_CircuitPython_MCP4725.svg?branch=master
:target: https://travis-ci.org/adafruit/Adafruit_CircuitPython_MCP4725
:alt: Build Status

CircuitPython module for the MCP4725 digital to analog converter.

Dependencies
Expand Down
35 changes: 16 additions & 19 deletions adafruit_mcp4725.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.
"""
`Adafruit_MCP4725`
====================================================
`adafruit_mcp4725` - MCP4725 digital to analog converter
========================================================

CircuitPython module for the MCP4725 digital to analog converter. See
examples/simpletest.py for a demo of the usage.
Expand All @@ -35,19 +35,17 @@


# Internal constants:
_MCP4725_DEFAULT_ADDRESS = const(0b01100010)
_MCP4725_DEFAULT_ADDRESS = 0b01100010
_MCP4725_WRITE_FAST_MODE = const(0b00000000)


class MCP4725:
"""MCP4725 12-bit digital to analog converter. This class has a similar
interface as the CircuitPython AnalogOut class and can be used in place
of that module. To construct pass the following parameters to the
initializer:
- i2c: The I2C bus.
interface as the CircuitPython AnalogOut class and can be used in place
of that module.

Optionally pass:
- address: The address of the device if set differently from the default.
:param ~busio.I2C i2c: The I2C bus.
:param int address: The address of the device if set differently from the default.
"""


Expand Down Expand Up @@ -99,11 +97,12 @@ def _read(self):

@property
def value(self):
"""Get and set the DAC value as a 16-bit unsigned value compatible
with the AnalogOut class. Note that the MCP4725 is still just a 12-bit
device so quantization will occur. If you'd like to instead deal with
the raw 12-bit value use the raw_value property, or the normalized_value
property to deal with a 0...1 float value.
"""The DAC value as a 16-bit unsigned value compatible with the
:py:class:`~analogio.AnalogOut` class.

Note that the MCP4725 is still just a 12-bit device so quantization will occur. If you'd
like to instead deal with the raw 12-bit value use the raw_value property, or the
normalized_value property to deal with a 0...1 float value.
"""
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an email thread with @tdicola:

why the change in language to remove "get and set" for property descriptions?  That's an explicit
decision to make it clear what properties are read-only vs. read-write.  Many of the sensors have
a mix of read-only (acceleration, light, etc.) and read/write properties (gain, data_rate, etc.) so
the documentation is being explicit about what is readable and writable.  Unfortunately RTD
doesn't add this itself even though it's state that Python could presumably parse and infer.

Phrases like "Get or set" and "return" are confusing because they are actions similar to how functions would be described. (Here is an example)It feels ok to say this as the implementer because you know its a function under the hood. However, from the outside, properties are more similar to variables because they represent state. Reading and writing variables is implicit in Python so its unnecessary to call it out.

You make a good point about read-only and I'd suggest using that phrase instead. So for example, DAC value as a 16-bit unsigned value compatible with the AnalogOut class. **Read-only** For read/write properties I think its clearer to omit it than include it. Starting a bunch of properties with the phrase "get or set" or similar is distracting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I don't follow the issue with the language. This is consistent with the API docs we have today, for example DigitalInOut: http://circuitpython.readthedocs.io/en/latest/shared-bindings/digitalio/DigitalInOut.html "Get or set the pin drive mode.", "Get or set the pin pull", etc. AnalogIn: http://circuitpython.readthedocs.io/en/latest/shared-bindings/analogio/AnalogIn.html#analogio.AnalogIn.value "The returned value..." etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way to say it is that attributes (whether properties, class attributes or instance attributes) don't do anything on their own so verbs like get, set or return are inappropriate. Instead, attributes are labels for a piece of state and Python provides separate syntax to manipulate that state. The comment should reflect what that state is not what can be done to it.

We have been inconsistent about this in the past and I'd be happy to fix those places as well. MicroPython has lots of "Get or set" comments for their setter/getter functions that we have inherited.

Looking more broadly at how attributes are documented across Python packages I don't see any that say "get or set". Here is where I looked:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this isn't actually state though, it's an I2C transaction that's reading a light level channel from the device. We might give the illusion that it's some state or attribute on an object, but it ultimately is not and can have subtle differences in behavior that need to be documented. For example these light level values are not mutable but the gain values are mutable--the documentation needs to be clear what is readable and what is writable. Removing language like "get" or "set" makes this more difficult to understand. How should a light level that can only be read be described without using the words "get" or "read", or a gain that be both written and read be described without "set", "write", or other verbs?

It's great to look at other documentation but I see plenty of things in those docs that are ambiguous here too:

  • Python datetime is explicit about read-only properties--look at the time object, it calls out a separate list of read-only properties and uses the verb "read". Some descriptions even call out the function they're calling internally (utc is described as "timezone(timedelta(0))").
    Subprocess is explicit about read-write vs. read-only (popen.stdin, popen.stdout, etc.).
  • dtype by design is immutable so the docs don't have a problem of calling out specific differences (most of numpy is immutable state beyond the ndarray). Look at scipy classes that have mutable state, for example the imageio module that replaced imread: http://imageio.readthedocs.io/en/latest/userapi.html#imageio.core.format.Reader Or look at the most popular image library, PIL/pillow: https://pillow.readthedocs.io/en/latest/reference/Image.html#the-image-class It makes heavy use of get names, "read" verbs, etc. and is very explicit about read-only vs. read-write.
  • Flask has APIs like "set_cookie", "get_json", "get_send_file_max_age", etc. Requests has "get_adapter", "get_redirect_target", etc.

This could go on and on looking at different styles, etc. but isn't really productive to do so. In my mind the most important thing is consistency--if our docs today use language like "get" and "set" then I don't see why just this library needs to be the focus of a change. Let's decide on a language and make it a universal change across all the docs. I'm happy to change language but it needs to be clear what is readable and what is readable & writable state to prevent user confusion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My decision is to omit verbs from property comments because they are used and documented like attributes. I've been asking for this change on other PRs I've done in the past so some libraries are already done this way. I'm sorry I didn't provide this feedback to you sooner.

Borrowing from this style guide lets use (read-only) and (write-only) at the end of property comments where they are needed. read-write properties are most common so lets omit any additional designation in that case.

I'll follow up this morning with additional explanation in the design guide and fixes for the core docs you pointed out.

raw_value = self._read()
# Scale up to 16-bit range.
Expand All @@ -118,9 +117,8 @@ def value(self, val):

@property
def raw_value(self):
"""Get and set the DAC value as a 12-bit unsigned value. This is the
the true resolution of the DAC and will never peform scaling or run
into quantization error.
"""The DAC value as a 12-bit unsigned value. This is the the true resolution of the DAC
and will never peform scaling or run into quantization error.
"""
return self._read()

Expand All @@ -130,8 +128,7 @@ def raw_value(self, val):

@property
def normalized_value(self):
"""Get and set the DAC value as a floating point number in the range
0 to 1.0.
"""The DAC value as a floating point number in the range 0.0 to 1.0.
"""
return self._read()/4095.0

Expand Down
2 changes: 1 addition & 1 deletion conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
# Uncomment the below if you use native CircuitPython modules such as
# digitalio, micropython and busio. List the modules you use. Without it, the
# autodoc module docs will fail to generate with a warning.
# autodoc_mock_imports = ["digitalio", "busio"]
autodoc_mock_imports = ["micropython"]

intersphinx_mapping = {'python': ('https://docs.python.org/3.4', None),'BusDevice': ('https://circuitpython.readthedocs.io/projects/bus_device/en/latest/', None),'CircuitPython': ('https://circuitpython.readthedocs.io/en/latest/', None)}

Expand Down