bobjacobsen / python-openlcb

MIT License
2 stars 1 forks source link

Standardize structure #7

Closed Poikilos closed 8 months ago

Poikilos commented 8 months ago

TLDR: PEP8 is widely used as a coding style and can help reduce diffs, make code appear more standardized and reduce the output of static analyzers so useful information can be discovered. Doing so led to some fixes either listed below or in individual commit descriptions.

The Python project itself has placed an emphasis on PEP8 (pycodestyle (formerly pep8) package helps lint that specifically), possibly since it is an interpreted language (where static analysis in general can prevent issues). I added IDE notes for highlighting in realtime (some realtime linting enabled by default in some IDEs, so conforming reduces noise).

I also started adding Google-style Sphinx docstrings (PEP8 specifies using a docstring, but Sphinx has a specific tiered formatting and Google-style provides the most specificity regarding types, returns, etc.). For example:

    def segmentAddressedDataArray(self, alias, data):
        '''Segment data into zero or more arrays
        of no more than 8 bytes, with the alias at the start of each,
        for addressed non-datagram messages.

        Args:
            alias (int): _description_
            data (_type_): _description_

        Returns:
            _type_: _description_
        '''

The classes or init but not both can document arguments. Using the class is helpful to reduce work (provide only one description of what using the class does), so I prefer placing init args in the class definition:

class MemoryReadMemo :
    """Memo carries request and reply.

    Args:
        nodeID (_type_): _description_
        size (_type_): _description_
        space (_type_): _description_
        address (_type_): _description_
        rejectedReply (_type_): _description_
        dataReply (_type_): _description_
    """
    def __init__(self, nodeID, size, space, address, rejectedReply, dataReply):
        # For args see class docstring.

You can also document which exceptions your code may raise:

        Raises:
            ValueError: _description_ [in what case, etc.]

Some PEP8 issues solved:

Below are diffs for behavior changes not mentioned in commit descriptions so that reviewing the changes below can be double-checked):

-                    aliasToNodeID[destAlias] = destID
-                    nodeIdToAlias[destID] = destAlias
+                    self.aliasToNodeID[destAlias] = destID
+                    self.nodeIdToAlias[destID] = destAlias
-                logging.warning("Did not know source = {} on message send".format(source))
+                logging.warning("Did not know source = {} on message send".format(msg.source))
-        lmsg = Message(MTI.Link_Layer_Restarted, NodeID(0), None, [])
+        msg = Message(MTI.Link_Layer_Restarted, NodeID(0), None, [])
        self.fireListeners(msg)
-                if destt == None:
-                    dest = NodeID(0)
+                if destt is None:
+                    destt = NodeID(0)

and in memoryservice.py:

bobjacobsen commented 8 months ago

Thanks! This is an amazing amount of improvement.