Skip to content

added auto_write #14

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

Jump to bottom
Merged
merged 4 commits into from
Jun 26, 2018
Merged

added auto_write #14

merged 4 commits into from
Jun 26, 2018

Conversation

caternuson
Copy link
Contributor

Adds an auto write feature per #13

@caternuson caternuson requested a review from a team June 23, 2018 00:10
@tannewt
Copy link
Member

tannewt commented Jun 25, 2018

@kattni can you review this?

sommersoft
sommersoft requested changes Jun 25, 2018
View reviewed changes
Copy link
Collaborator

@sommersoft sommersoft left a comment

Choose a reason for hiding this comment

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

Just documentation requests. Everything looks good code-wise.

In addition to the inline requests, the README.rst example needs to be updated.

adafruit_ht16k33/ht16k33.py Outdated
@@ -40,10 +40,12 @@

class HT16K33:
"""The base class for all displays. Contains common methods."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a note here that auto_write is on by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that just to make it more obvious than what Read The Docs will have? Otherwise, it's similar to the default value for address.

examples/matrix.py
matrix.fill(0)
matrix.show()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe leave this in, but commented out, denoting that it is required if auto_write == False?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if I agree with this. I can see where @sommersoft is coming from, but I think it can start bloating the simple examples to include all the potentially necessary code if you changed the default kwargs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had something in there, but then took it out for same reason @kattni mentions. I think this is what I had, how about this:

matrix.fill(0)
# If you turn auto_write off, you will need to call show() to update the display
# matrix.show()

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can agree with the bloat concern. Having the parameters in the API documentation is enough for me.

examples/segments.py
display.fill(0)
display.show()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above

kattni
kattni suggested changes Jun 25, 2018
View reviewed changes
Copy link
Contributor

@kattni kattni left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I have a question regarding documentation - it may or may not be applicable in this case. Please review it.

adafruit_ht16k33/ht16k33.py Outdated
@@ -40,10 +40,12 @@

class HT16K33:
"""The base class for all displays. Contains common methods."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a base class for multiple things, I'm not sure it's applicable to expand this docstring to the same extent as, for example, the DotStar lib: https://github.com/adafruit/Adafruit_CircuitPython_DotStar/blob/af25424ee7dbebea3e5d77390c017018ffa52d36/adafruit_dotstar.py#L51

If it is applicable, we should consider doing it. If it's not, please disregard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and put in the :param doc. That seems to make sense, and could take care of @sommersoft 's request for documenting auto_write default.

kattni
kattni approved these changes Jun 26, 2018
View reviewed changes
Copy link
Contributor

@kattni kattni left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for the updates!

@sommersoft sommersoft merged commit b545d77 into adafruit:master Jun 26, 2018
@sommersoft sommersoft mentioned this pull request Jun 26, 2018
Closed
@caternuson caternuson deleted the iss13 branch June 26, 2018 21:04
tannewt pushed a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Aug 7, 2018
@adafruit-adabot
Automated update by Adabot (adafruit/adabot@e893f58)
c2a61bd 
Updating https://github.com/adafruit/Adafruit_CircuitPython_GPS to 3.1.0 from 3.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_GPS#7 from kattni/pypi
  > updated CoC
  > Merge pull request adafruit/Adafruit_CircuitPython_GPS#6 from margaret/issue_5
  > Merge pull request adafruit/Adafruit_CircuitPython_GPS#4 from sommersoft/new_docs

Updating https://github.com/adafruit/Adafruit_CircuitPython_HT16K33 to 2.1.0 from 2.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#19 from kattni/pypi
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#18 from caternuson/iss17
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#16 from caternuson/iss7
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#15 from caternuson/iss13b
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#14 from caternuson/iss13
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#12 from caternuson/iss3
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#11 from caternuson/iss3
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#10 from elmwoodelec/master
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#6 from sommersoft/new_docs

Updating https://github.com/adafruit/Adafruit_CircuitPython_IRRemote to 3.3.0 from 3.2.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_IRRemote#15 from kattni/pypi
  > Merge pull request adafruit/Adafruit_CircuitPython_IRRemote#14 from kattni/add-tx-example

Updating https://github.com/adafruit/Adafruit_CircuitPython_IS31FL3731 to 2.2.0 from 2.1.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_IS31FL3731#11 from kattni/pypi
  > updated CoC
  > Merge pull request adafruit/Adafruit_CircuitPython_IS31FL3731#10 from sommersoft/new_docs

Updating https://github.com/adafruit/Adafruit_CircuitPython_L3GD20 to 1.1.0 from 1.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_L3GD20#4 from kattni/pypi
  > updated CoC
  > Merge pull request adafruit/Adafruit_CircuitPython_L3GD20#3 from mrmcwethy/fixdocs

Updating https://github.com/adafruit/Adafruit_CircuitPython_LSM303 to 1.2.0 from 1.1.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_LSM303#5 from kattni/pypi
  > updated CoC
  > Merge pull request adafruit/Adafruit_CircuitPython_LSM303#4 from sommersoft/new_docs

Updating https://github.com/adafruit/Adafruit_CircuitPython_LSM9DS0 to 1.1.0 from 1.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_LSM9DS0#5 from kattni/pypi
  > updated CoC
  > Merge pull request adafruit/Adafruit_CircuitPython_LSM9DS0#3 from sommersoft/new_docs

Updating https://github.com/adafruit/Adafruit_CircuitPython_LSM9DS1 to 1.1.0 from 1.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_LSM9DS1#7 from kattni/pypi
  > updated CoC
  > Merge pull request adafruit/Adafruit_CircuitPython_LSM9DS1#5 from sommersoft/new_docs
  > Merge pull request adafruit/Adafruit_CircuitPython_LSM9DS1#3 from sommersoft/new_docs

Updating https://github.com/adafruit/Adafruit_CircuitPython_MatrixKe to 1.1.1 from 1.0.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_MatrixKe#2 from kattni/badge-fix
  > Merge pull request adafruit/Adafruit_CircuitPython_MatrixKe#1 from kattni/pypi
  > updated CoC

Updating https://github.com/adafruit/Adafruit_CircuitPython_MAX31855 to 3.0.3 from 3.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_MAX31855#7 from kattni/fixes

Updating https://github.com/adafruit/Adafruit_CircuitPython_MAX31865 to 2.1.0 from 2.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_MAX31865#6 from kattni/pypi
  > updated CoC

Updating https://github.com/adafruit/Adafruit_CircuitPython_MAX7219 to 1.1.0 from 1.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_MAX7219#13 from kattni/lint-fix
  > Merge pull request adafruit/Adafruit_CircuitPython_MAX7219#12 from kattni/pypi
  > updated CoC
  > Merge pull request adafruit/Adafruit_CircuitPython_MAX7219#11 from sommersoft/new_docs

Updating https://github.com/adafruit/Adafruit_CircuitPython_MAX9744 to 1.1.0 from 1.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_MAX9744#6 from kattni/pypi
  > updated CoC
  > Merge pull request adafruit/Adafruit_CircuitPython_MAX9744#5 from sommersoft/new_docs
  > Merge pull request adafruit/Adafruit_CircuitPython_MAX9744#4 from sommersoft/new_docs

Updating https://github.com/adafruit/Adafruit_CircuitPython_MCP2 to 1.1.0 from 1.0.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_MCP2#3 from kattni/pypi
  > updated CoC
  > attack of the typo
  > Get ready to PyPI
  > Merge pull request adafruit/Adafruit_CircuitPython_MCP2#2 from adafruit/deshipu-patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_MCP4725 to 1.1.0 from 1.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_MCP4725#5 from kattni/pypi
  > updated CoC

Updating https://github.com/adafruit/Adafruit_CircuitPython_MCP9808 to 3.2.0 from 3.1.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_MCP9808#14 from kattni/pypi
  > updated CoC

Updating https://github.com/adafruit/Adafruit_CircuitPython_MMA8451 to 1.1.0 from 1.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_MMA8451#2 from kattni/pypi
  > updated CoC
  > Merge pull request adafruit/Adafruit_CircuitPython_MMA8451#1 from jepler/import-struct

Updating https://github.com/adafruit/Adafruit_CircuitPython_MPL3115A2 to 1.1.0 from 1.0.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_MPL3115A2#3 from kattni/pypi
  > updated CoC

Updating https://github.com/adafruit/Adafruit_CircuitPython_MPR121 to 1.1.0 from 1.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_MPR121#7 from kattni/pypi
  > updated CoC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kattni kattni kattni approved these changes

@sommersoft sommersoft sommersoft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@caternuson @tannewt @kattni @sommersoft