adafruit / Adafruit_CircuitPython_HT16K33

Adafruit CircuitPython driver for the HT16K33, a LED matrix driver IC.
MIT License
41 stars 29 forks source link

Port changes to the number() routine in the micropython library to _number() #53

Closed hybotics closed 4 years ago

hybotics commented 4 years ago

This is a port of the changes I did to the number() routine in the micropython version of this library. I also found several errors in processing floats and negative numbers, which I fixed. This version is much more robust. I also fixed a problem with the "from adafruit_ht16k33 import..." line.

hybotics commented 4 years ago

PyLint does not like me.

hybotics commented 4 years ago

Done.

hybotics commented 4 years ago

Hi,

I have read your comments and I have a question.

if a and b: print("True")

will be True if a is positive and b is negative or vice versa. This would introduce a bug if for some reason one of them is negative and you really want to check for both > 0. This is why I explicitly check for > 0. I can drop the parentheses and everything will still come still be right.

What do you think?

On Thu, Jan 23, 2020 at 1:59 PM Melissa LeBlanc-Williams < notifications@github.com> wrote:

@makermelissa requested changes on this pull request.

Thanks for submitting this. I've made some suggested changes that should help with it passing linting. I'm not sure why you changed it to import from matrix which imports from the base class (ht16k33) rather than just importing the ht16k33 class for better memory usage.

In adafruit_ht16k33/segments.py https://github.com/adafruit/Adafruit_CircuitPython_HT16K33/pull/53#discussion_r370373344 :

@@ -26,7 +26,8 @@ """

from time import sleep -from adafruit_ht16k33.ht16k33 import HT16K33 +#from adafruit_ht16k33.ht16k33 import HT16K33 +from adafruit_ht16k33.matrix import HT16K33

Why was this changed from the base class to the 8x8 matrix class?

In adafruit_ht16k33/segments.py https://github.com/adafruit/Adafruit_CircuitPython_HT16K33/pull/53#discussion_r370373873 :

  • places += 1
  • self._text(string[:places])
  • stnum = str(number)
  • dot = stnum.find('.')
  • if ((len(stnum) > 5) or ((len(stnum) > 4) and (dot < 0))):
  • raise ValueError("Input overflow - {0} is too large for the display!".format(number))
  • if dot < 0:
  • No decimal point (Integer)

  • places = len(stnum)
  • else:
  • places = len(stnum[:dot])
  • if self.debug:
  • print("(1) number = {0}, places = {1}, decimal = {2}, dot = {3}, stnum = '{4}'".format(number, places, decimal, dot, stnum))

I'd suggest removing the debug statements. They're long and make the library larger. They also seem to be the source of a lot of the linting problems.

In adafruit_ht16k33/segments.py https://github.com/adafruit/Adafruit_CircuitPython_HT16K33/pull/53#discussion_r370375054 :

  • '''
  • Display a floating point or integer number on the Adafruit HT16K33 based displays
  • Param: number - The floating point or integer number to be displayed, which must be
  • in the range 0 (zero) to 9999 for integers and floating point or integer numbers
  • and between 0.0 and 999.0 or 99.00 or 9.000 for floating point numbers.
  • Param: decimal - The number of decimal places for a floating point number if decimal
  • is greater than zero, or the input number is an integer if decimal is zero.
  • Returns: The output text string to be displayed.'''

This docstring should be moved into the correct function.

In adafruit_ht16k33/segments.py https://github.com/adafruit/Adafruit_CircuitPython_HT16K33/pull/53#discussion_r370376446 :

+

  • if self.debug:
  • print("(1) number = {0}, places = {1}, decimal = {2}, dot = {3}, stnum = '{4}'".format(number, places, decimal, dot, stnum))
  • if ((places <= 0) and (decimal > 0)):
  • self.fill(False)
  • places = 4
  • if '.' in stnum:
  • places += 1
  • if self.debug:
  • print("(2) places = {0}, dot = {1}, decimal = {2}, stnum = '{3}'".format(places, dot, decimal, stnum))
  • Set decimal places, if number of decimal places is specified (decimal > 0)

  • if ((places > 0) and (decimal > 0) and (dot > 0) and (len(stnum[places:]) > decimal)):

This is failing pylint because of all the parenthesis and variable > 0 can be shortened to variable. I'd suggest changing this line to: if places and decimal and dot and len(stnum[places:]) > decimal:

In adafruit_ht16k33/segments.py https://github.com/adafruit/Adafruit_CircuitPython_HT16K33/pull/53#discussion_r370376655 :

  • stnum = str(number)
  • dot = stnum.find('.')
  • if ((len(stnum) > 5) or ((len(stnum) > 4) and (dot < 0))):
  • raise ValueError("Input overflow - {0} is too large for the display!".format(number))
  • if dot < 0:
  • No decimal point (Integer)

  • places = len(stnum)
  • else:
  • places = len(stnum[:dot])
  • if self.debug:
  • print("(1) number = {0}, places = {1}, decimal = {2}, dot = {3}, stnum = '{4}'".format(number, places, decimal, dot, stnum))
  • if ((places <= 0) and (decimal > 0)):

This is failing pylint because of the parenthesis and variable > 0 can be shortened to variable. I'd suggest changing this line to: if places <= 0 and decimal:

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/adafruit/Adafruit_CircuitPython_HT16K33/pull/53?email_source=notifications&email_token=AA5DR24VAZDJHQANK5KNMU3Q7IHL3A5CNFSM4KKNPB2KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCS4HA2A#pullrequestreview-347631720, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA5DR2ZX2RLUNL2IPLWI4SLQ7IHL3ANCNFSM4KKNPB2A .

makermelissa commented 4 years ago

Ok yeah, that makes sense. If pylint fails on that, you can add lines to disable certain checks with #pylint: disable-msg=[pylint-fail-code] and replace [pylint-fail-code] with the actual fail code.

hybotics commented 4 years ago

Done! I will get better as I learn what PyLint expects.