First pass at adding type annotations #32

prcutler · 2022-10-17T16:20:14Z

This is my first time updating a library for type annotations so it could probably use a thorough review.

I'm also stuck on the following types, if anyone has some feedback or direction for me:

Line 100: type for spi_bus
Line 125: type for file_name

I'm happy to update the PR with the right types or fix any errors.

Thanks!

tekktrik

Nice work! I added feedback below, but more generally it looks like pre-commit has some feedback about the general formatting and linting. The formatting is easily fixed by running it as a command line tool (`--pre-commit run --all-files will do fine) and committing those changes. I've added the linting changes along with the other comments!

I tried to be thorough in the explanations of changes for context, but let me know if anything is unclear, or if more information would be helpful!

tekktrik · 2022-10-23T23:25:03Z

adafruit_avrprog.py

@@ -32,8 +32,16 @@
 __version__ = "0.0.0+auto.0"
 __repo__ = "https://github.com/adafruit/Adafruit_CircuitPython_AVRprog.git"

+import digitalio


If you're just using something for typing like this library or the microcontroller import below, you can put that in the try/except block guarded by a first import of typing. The logic is as follows:

CircuitPython actually doesn't care about type annotation syntax as a method of saving space, and likewise doesn't store the information gleaned by parsing it. The effect is that it's basically anything goes, even having unbound types (using a variable that hasn't been assigned). Note that this only works in CircuitPython - CPython does care, and will get very mad if it can't resolve a type annotation.

So, in order to save RAM on the microcontroller, we avoid import items that only get used as a part of type annotation since that information isn't used or stored anyway. The way this is achieved is by using the typing import - if we're in a microcontroller environment, it fails and skips importing anything else in the block. But if it's Blinka, it will succeed and continue to import anything else in that block that is required for type annotations.

So! Since this and the following are in that category of inputs, let's put them after the import of typing in that block.

tekktrik · 2022-10-23T23:27:31Z

adafruit_avrprog.py

 from digitalio import Direction, DigitalInOut

+try:
+    import typing


pylint warns against unused imports, since it figures you can just remove it. However, this goes against our strategy of using it as a sort of import guard, so in cases where we're actually not importing anything from typing, we can disable the pylint warning with a comment it parses:

import typing # pylint: disable=unused-import

However, I have some suggestion below that will make use of things imported from typing, so we won't need to use that strategy in the end.

tekktrik · 2022-10-23T23:29:42Z

adafruit_avrprog.py

@@ -89,7 +97,7 @@ class Boards:
    _spi = None
    _rst = None

-    def init(self, spi_bus, rst_pin):
+    def init(self, spi_bus, rst_pin: microcontroller.Pin) -> None:


spi_bus is type busio.SPI, the tell is how it can use the configure() method, and busio imports are common for the drivers.

tekktrik · 2022-10-23T23:39:14Z

adafruit_avrprog.py

@@ -100,7 +108,7 @@ def init(self, spi_bus, rst_pin):
        self._rst.direction = Direction.OUTPUT
        self._rst.value = True

-    def verify_sig(self, chip, verbose=False):
+    def verify_sig(self, chip: dict, verbose: bool = False) -> bool:


The typing for chip is correct and valid, but Python also allows for the use of generics, which allow you to specify the content of some types. In this case, we can specify the types of the keys and values of the dict.

To do this for Python 3.7+ (earliest Python supported by Blinka), we can import Dict from the typing module and use brackets to specify the key and value types. You can see an example of a dict that gets passed in here:

https://github.com/prcutler/Adafruit_CircuitPython_AVRprog/blob/daf67694dbbb4a0d4394d8e0c1439e261cdc03da/adafruit_avrprog.py#L60

All of the keys are str, so that will be that. For the values, it's a little trickier. We could use a Union to say it can take str, list, etc., but that's a bit of a clutter. The other option is to specify values as typing.Any which is kinda like saying "look, don't count on this being any specific type, it could be anything at all, so don't try to lint it or try to use it unless you know what it is". That's probably a good fit for this use case.

With all of that said, after import Dict and Any from typing, we can type it as:

Suggested change

def verify_sig(self, chip: dict, verbose: bool = False) -> bool:

def verify_sig(self, chip: Dict[str, Any], verbose: bool = False) -> bool:

I won't comment for every instance where this applies but you can do this for all of them!

tekktrik · 2022-10-23T23:41:37Z

adafruit_avrprog.py

@@ -114,7 +122,7 @@ def verify_sig(self, chip, verbose=False):
            return False
        return True

-    def program_file(self, chip, file_name, verbose=False, verify=True):
+    def program_file(self, chip: dict, file_name, verbose=False, verify=True) -> bool:


Looking at one of the examples, it looks like filename is type str. Don't forget to add verbose and verify as well!

tekktrik · 2022-10-23T23:56:25Z

adafruit_avrprog.py

@@ -262,7 +270,7 @@ def write_fuses(self, chip, low=None, high=None, ext=None, lock=None):

    # pylint: enable=unused-argument,expression-not-assigned

-    def verify_fuses(self, chip, low=None, high=None, ext=None, lock=None):
+    def verify_fuses(self, chip: dict, low=None, high=None, ext=None, lock=None) -> bool:


Looks like the other arguments can be Optional[int] here as well.

tekktrik · 2022-10-23T23:59:44Z

adafruit_avrprog.py

        """
        End programming mode: SPI is released, and reset pin set high.
        """
        self._spi.unlock()
        self._rst.value = True

-    def read_signature(self):
+    def read_signature(self) -> list:


We can also define the contents of a list using generics. For lists, we only need to define what any given element can be. The return from _transaction() is bytearray, and an element of a bytearray is an int. Since we're appending only int to the list, we can import List from `typing and type the return here as:

Suggested change

def read_signature(self) -> list:

def read_signature(self) -> List[int]:

tekktrik · 2022-10-24T00:02:23Z

adafruit_avrprog.py

@@ -317,7 +325,7 @@ def read_signature(self):
            sig.append(self._transaction((0x30, 0, i, 0))[2])
        return sig

-    def read(self, addr, read_buffer):
+    def read(self, addr: int, read_buffer: bytes) -> None:


Since we're writing things into what's provided into read_buffer, we'll need to use a type that can support writing using the indices. In this case, let's use bytearray, which supports that. Sometimes we can use an even less specific type, but given how it's used, I do think that only bytearray will work.

tekktrik · 2022-10-24T00:04:25Z

adafruit_avrprog.py

@@ -345,7 +353,7 @@ def _flash_word(self, addr, low, high):
        self._transaction((0x40, addr >> 8, addr, low))
        self._transaction((0x48, addr >> 8, addr, high))

-    def _flash_page(self, page_buffer, page_addr, page_size):
+    def _flash_page(self, page_buffer: float, page_addr: int, page_size: int):


Looks like page_buffer needs to support access by indices and that individual elements are int. Looking at how it gets used in the internals, it looks like it expects a bytearray. You can also add the None return type!

tekktrik · 2022-10-24T00:05:04Z

adafruit_avrprog.py

@@ -375,7 +383,7 @@ def _busy_wait(self):
            pass


-def read_hex_page(file_state, page_addr, page_size, page_buffer):
+def read_hex_page(file_state: dict, page_addr: int, page_size: int, page_buffer: bytearray):


You can add the return type here as well, which appears to be bool.

tekktrik

Whoops, meant to click "Request changes"!

FoamyGuy · 2024-05-06T20:13:43Z

annotations are now merged in from #34

First pass at adding type annotations

daf6769

tekktrik reviewed Oct 24, 2022

View reviewed changes

tekktrik requested changes Oct 24, 2022

View reviewed changes

FoamyGuy added the hacktoberfest-accepted label Oct 31, 2022

FoamyGuy closed this May 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

First pass at adding type annotations #32

First pass at adding type annotations #32

Uh oh!

prcutler commented Oct 17, 2022

Uh oh!

tekktrik left a comment

Uh oh!

tekktrik Oct 23, 2022

Uh oh!

tekktrik Oct 23, 2022

Uh oh!

tekktrik Oct 23, 2022

Uh oh!

tekktrik Oct 23, 2022

Uh oh!

tekktrik Oct 23, 2022

Uh oh!

tekktrik Oct 23, 2022

Uh oh!

tekktrik Oct 23, 2022

Uh oh!

tekktrik Oct 24, 2022

Uh oh!

tekktrik Oct 24, 2022

Uh oh!

tekktrik Oct 24, 2022

Uh oh!

tekktrik left a comment

Uh oh!

FoamyGuy commented May 6, 2024

Uh oh!

Uh oh!

	def verify_sig(self, chip: dict, verbose: bool = False) -> bool:
	def verify_sig(self, chip: Dict[str, Any], verbose: bool = False) -> bool:

	def read_signature(self) -> list:
	def read_signature(self) -> List[int]:

First pass at adding type annotations #32

First pass at adding type annotations #32

Uh oh!

Conversation

prcutler commented Oct 17, 2022

Uh oh!

tekktrik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tekktrik left a comment

Choose a reason for hiding this comment

Uh oh!

FoamyGuy commented May 6, 2024

Uh oh!

Uh oh!