Add type hints to the whole project and mypy testing setup and CI #123

jeffkala · 2022-06-24T13:56:32Z

The code base was previously mixed, some stuff was type hinted other stuff wasn't. Went ahead got everything up to date. Quite a few ignores are in place for now but can be cleaned up over time.

itdependsnetworks · 2022-06-26T15:16:28Z

Pretty cool stuff!! Will ping @Kircheneer to ask for an extra pair of eyes.

Kircheneer · 2022-06-27T07:01:10Z

Brilliant stuff @jeffkala!

I have a couple of meta-level comments and will dive into the down and dirty in a moment.

As per this and DRY I believe we should drop type annotations from the docstrings wherever code-level type hints are present
If we add an empty file called py.typed to the python module root other modules importing netutils can also benefit from its type hinting (see here)
Is full type hinting something we want to advertise as a "feature" and/or include in the library tenets?

EDIT:

And a couple after:

Most of these comments are concerned with cutting down on the number of ignoring comments. I have a couple of suggestions buried in these:

Introduce a NOS Enum to use as a type in a couple of these, could also prove useful to export this for importing from other projects that might need it (have netutils be somewhat of a source of truth for NOS naming?)
A couple of changes I suggested are for correct type hints where I think the addition of type hinting has revealed suboptimal interfaces, but I might be wrong here as I don't have the best picture of the entire codebase

Kircheneer · 2022-06-27T07:06:45Z

netutils/bandwidth.py

            if nbr_decimal == 0:
-                nbr_decimal = None
+                nbr_decimal = None  # type: ignore


I believe that round(x, None) == round(x) for all x, right? Meaning we could drop this branch and the corresponding # type: ignore.

Kircheneer · 2022-06-27T07:14:08Z

netutils/config/clean.py



-def clean_config(config, filters):
+def clean_config(config: str, filters: t.List[str]) -> str:


The docstring example for this doesn't pass type checking. Now AFAIK there is no way for Mypy to perform type checking on the code in the examples, but I still think it's worth pointing out: clean_filters in that example is a List[Dict[str, str]] rather than the List[str] annotation in the function signature here. Looking at the # type: ignore further down it appears the proper type for filters is that from the docstring (List[Dict[str, str]]).

Kircheneer · 2022-06-27T07:16:08Z

netutils/config/clean.py

@@ -48,11 +48,11 @@ def clean_config(config, filters):
        >>>
    """
    for item in filters:
-        config = re.sub(item["regex"], "", config, flags=re.MULTILINE)
+        config = re.sub(item["regex"], "", config, flags=re.MULTILINE)  # type: ignore


See last comment, can be removed if the function signature is fixed.

Kircheneer · 2022-06-27T07:18:40Z

netutils/config/clean.py

-    for item in filters:
-        config = re.sub(item["regex"], item["replace"], config, flags=re.MULTILINE)
+    for item in filters:  # type: ignore
+        config = re.sub(item["regex"], item["replace"], config, flags=re.MULTILINE)  # type: ignore


Same thing as with clean_config

Kircheneer · 2022-06-27T07:21:15Z

netutils/config/compliance.py

 from . import parser  # pylint: disable=relative-beyond-top-level

-parser_map = {
+parser_map: t.Dict[str, t.Callable] = {  # type: ignore


Type of parser_map should be t.Dict[str, t.Type[BaseConfigParser]]. t.Type[SomeClass] is the type hint for the actual class type or any subclasses (rather than instances of the class).

Kircheneer · 2022-06-27T07:23:37Z

netutils/config/compliance.py

@@ -15,7 +17,7 @@
    "nokia_sros": parser.NokiaConfigParser,
 }

-default_feature = {
+default_feature: t.Dict[str, t.Union[str, bool, None]] = {


Would it possibly make sense to use a TypedDict here? It seems like default_feature is somewhat struct-esque, with a TypedDict we could clearly identify the types of all its values.

Kircheneer · 2022-06-27T07:24:22Z

netutils/config/compliance.py

@@ -27,7 +29,7 @@
 }


-def _check_configs_differences(intended_cfg, actual_cfg, network_os):
+def _check_configs_differences(intended_cfg: str, actual_cfg: str, network_os: str) -> t.Dict[str, t.Union[str, bool]]:


If I see this correctly this would be another place we could use the proposed default_feature TypedDict.

Kircheneer · 2022-06-27T07:26:21Z

netutils/config/compliance.py

+def _open_file_config(cfg_path: str) -> t.Union[str, bool]:
    """Open config file from local disk."""
    try:
        with open(cfg_path, encoding="utf-8") as filehandler:
            device_cfg = filehandler.read()
    except IOError:
-        return False
+        return False  # This should probably be changed to a exception raise. Causing mypy issues on L183, L184
    return device_cfg.strip()


+1 on the exception part. The return type for this looks weird and would introduce lots of unnecessary checks because what the user of this function probably wants is the str. If something points to an exception not being correct here, how about an empty string?

Kircheneer · 2022-06-27T07:26:47Z

netutils/config/compliance.py

    return device_cfg.strip()


-def compliance(features, backup, intended, network_os, cfg_type="file"):
+def compliance(
+    features: t.List[t.Dict[str, t.Union[str, bool, t.List[str]]]],


Another possible place to use the default_feature TypedDict?

Kircheneer · 2022-06-27T07:27:21Z

netutils/config/compliance.py

+    backup: str,
+    intended: str,
+    network_os: str,
+    cfg_type: t.Optional[str] = "file",


If we supply a default argument for this but make the type Optional, do we expect users to explicitly pass None to cfg_type or can we just omit the Optional?

Kircheneer · 2022-06-27T07:28:59Z

netutils/config/compliance.py

+        backup_str = section_config(feature, backup_cfg, network_os)  # type: ignore
+        intended_str = section_config(feature, intended_cfg, network_os)  # type: ignore


This could be fixed by changing the return type of open_file_config to just a str.

Kircheneer · 2022-06-27T07:30:31Z

netutils/config/compliance.py



-def config_section_not_parsed(features, device_cfg, network_os):
+def config_section_not_parsed(
+    features: t.List[t.Dict[str, t.Union[str, bool, t.List[str]]]], device_cfg: str, network_os: str


Do we want network_os to be an Enum type? This way we further restrict possible values for it and make sure we don't accidentally pass wrong types to it. Also having netutils possibly even export such an Enum for use within importing libraries makes conceptual sense to me.

Kircheneer · 2022-06-27T07:31:57Z

netutils/config/compliance.py

@@ -216,11 +226,11 @@ def config_section_not_parsed(features, device_cfg, network_os):
        remaining_cfg = remaining_cfg.replace(feature_cfg, "")
    return {
        "remaining_cfg": remaining_cfg.strip(),
-        "section_not_found": section_not_found,
+        "section_not_found": section_not_found,  # type: ignore


Possible fix to build the feature TypedDict out and use it as the return value?

Kircheneer · 2022-06-27T07:35:28Z

netutils/config/compliance.py

@@ -418,7 +430,7 @@ def section_config(feature, device_cfg, network_os):
                continue
            else:
                match = False
-        for line_start in section_starts_with:
+        for line_start in section_starts_with:  # type: ignore


Could be fixed with the feature TypedDict as feature.get("section") would return something whose type Mypy would be able to infer correctly.

Kircheneer · 2022-06-27T07:36:39Z

netutils/config/parser.py

+    comment_chars: t.List[str] = ["!"]
+    banner_start: t.List[str] = ["banner", "vacant-message"]


These type hints are not needed, Mypy is able to infer this type correctly. I think we should drop them, as then they just add noise to the code.

Kircheneer · 2022-06-27T07:42:02Z

netutils/config/parser.py

+        self.build_config_relationship()  # type: ignore

-    def config_lines_only(self):
+    def config_lines_only(self):  # type: ignore
        """Remove lines not related to config."""
        raise NotImplementedError


How about:

class BaseConfigParser(metaclass=ABCMeta): ... def __init(self, config: str): ... self.generator_config: t.Generator[str, None, None] = (line for line in self.config_lines_only.splitlines()) ... self.build_config_relationship() @abstractmethod @property def config_lines_only(self) -> str: """Remove lines not related to config.""" pass @abstractmethod @property def banner_end(self) -> str: """Demarcate End of Banner char(s).""" pass @abstractmethod def build_config_relationship(self) -> t.List[ConfigLine]: """Parse text tree of config lines and their parents.""" pass

That should enable us to get rid of all the # type: ignore comments in this base class.

Kircheneer · 2022-06-27T07:46:59Z

netutils/config/parser.py

+    comment_chars: t.List[str] = ["!"]
+    banner_start: t.List[str] = ["banner", "vacant-message"]


Can also get rid of these annotations.

Kircheneer · 2022-06-27T07:47:14Z

netutils/config/parser.py

@@ -66,11 +67,11 @@ def is_banner_end(self, line):
        Returns:
            bool: True if line ends banner, else False.
        """
-        if self.banner_end in line:
+        if self.banner_end in line:  # type: ignore


Would be fixed by the proposed changes to the base class.

Kircheneer · 2022-06-27T07:49:34Z

netutils/config/parser.py

@@ -156,7 +157,7 @@ def get_leading_space_count(config_line):
        """
        return len(config_line) - len(config_line.lstrip())

-    def _remove_parents(self, line, current_spaces):
+    def _remove_parents(self, line: str, current_spaces: int) -> t.Tuple:  # type: ignore


t.Tuple[str, ...] as the return type should be able to get rid of the # type: ignore. Correspons to a tuple of varying size with str elements.

Kircheneer · 2022-06-27T07:50:32Z

netutils/config/parser.py

@@ -178,7 +179,7 @@ def _remove_parents(self, line, current_spaces):
        parents = self._current_parents[:-deindent_level] or (self._current_parents[0],)
        return parents

-    def _build_banner(self, config_line):
+    def _build_banner(self, config_line: str) -> t.Union[str, None]:


We could use t.Optional[str] here to simplify the type a little

Kircheneer · 2022-06-27T07:50:59Z

netutils/config/parser.py

@@ -198,7 +199,7 @@ def _build_banner(self, config_line):
            if not self.is_banner_end(line):
                banner_config.append(line)
            else:
-                line = normalise_delimiter_caret_c(self.banner_end, line)
+                line = normalise_delimiter_caret_c(self.banner_end, line)  # type: ignore


Fixed by proposed changes to the base class.

Kircheneer · 2022-06-27T08:14:34Z

netutils/interface.py

@@ -225,7 +239,7 @@ def abbreviated_interface_name(interface, addl_name_map=None, addl_reverse_map=N
        canonical_type = interface_type

    try:
-        abbreviated_name = rev_name_map[canonical_type] + str(interface_number)
+        abbreviated_name = rev_name_map[canonical_type] + str(interface_number)  # type: ignore


Resolvable by explicit optional handling further up.

Kircheneer · 2022-06-27T08:15:26Z

netutils/interface.py

@@ -237,7 +251,7 @@ def abbreviated_interface_name(interface, addl_name_map=None, addl_reverse_map=N
    return interface


-@total_ordering
+@total_ordering  # type: ignore


We could add this issue to this comment: python/mypy#5374

Kircheneer · 2022-06-27T08:15:54Z

netutils/interface.py

-    def __lt__(self, other) -> bool:  # noqa: D105
+    def __lt__(self, other) -> bool:  # type: ignore  # noqa: D105
        ...

-    def __eq__(self, other) -> bool:  # noqa: D105
+    def __eq__(self, other) -> t.Any:  # type: ignore  # noqa: D105
        return self.weight == other.weight and self.val == other.val


I believe we should only look at these once the bug with Mypy is addressed.

Kircheneer · 2022-06-27T08:16:38Z

netutils/interface.py

@@ -280,8 +294,8 @@ def __hash__(self) -> int:  # noqa: D105
 class CCString(CharacterClass):
    """Strings are sorted lexicographically."""

-    def __lt__(self, other) -> bool:  # noqa: D105
-        return self.weight < other.weight or self.val < other.val
+    def __lt__(self, other) -> bool:  # type: ignore  # noqa: D105


Can be fixed with def __lt__(self, other: "CharacterClass") -> bool: # noqa: D105

Kircheneer · 2022-06-27T08:17:08Z

netutils/interface.py

@@ -294,7 +308,7 @@ def weight(self) -> int:  # noqa: D107,D102
 class CCInt(CharacterClass):
    """Ints must be sorted canonically because '11' < '5'."""

-    def __lt__(self, other) -> bool:  # noqa: D105
+    def __lt__(self, other) -> bool:  # type: ignore  # noqa: D105


Can be fixed with def __lt__(self, other: "CharacterClass") -> bool: # noqa: D105

Kircheneer · 2022-06-27T08:17:48Z

netutils/interface.py

@@ -310,7 +324,7 @@ class CCSeparator(CharacterClass):

    weights: t.Dict[str, int] = {".": 10, "/": 20}

-    def __lt__(self, other) -> bool:  # noqa: D105
+    def __lt__(self, other) -> bool:  # type: ignore  # noqa: D105


Can be fixed with def __lt__(self, other: "CharacterClass") -> bool: # noqa: D105

Kircheneer · 2022-06-27T08:18:51Z

netutils/interface.py

@@ -321,7 +335,7 @@ def weight(self) -> int:  # noqa: D102
        return 30


-def _CCfail(*args):  # pylint: disable=C0103
+def _CCfail(*args) -> None:  # type: ignore  # pylint: disable=C0103


Should return NoReturn

Kircheneer · 2022-06-27T08:22:20Z

netutils/interface.py

-                tail = (*tail, cls(part, True))
+                tail = (*tail, cls(part, True))  # type: ignore
                break
            if part:
-                tail = (*tail, cls(part))
+                tail = (*tail, cls(part))  # type: ignore


There probably is a way to resolve these, but I deem it not worth it for now. Something like Union[Callable[[str, Optional[bool]], CharacterClass], Callable[[...], NoReturn] might do the trick if you want to play around with it a little.

Kircheneer · 2022-06-27T08:22:54Z

netutils/interface.py

+    addl_reverse_map: t.Optional[t.Dict[str, str]] = None,
+    verify: t.Optional[bool] = False,
+    order: t.Optional[str] = None,
+    reverse: t.Optional[bool] = None,


Does this ever need the None value, i.e. does it make a difference if its just False?

Kircheneer · 2022-06-27T08:23:47Z

netutils/interface.py

@@ -500,15 +519,15 @@ def abbreviated_interface_name_list(  # pylint: disable=R0913, R0914
        raise ValueError(f"Verify interface on and no match found for {no_match_string}")

    if order:
-        abbreviated_interface_list = INTERFACE_LIST_ORDERING_OPTIONS.get(order)(abbreviated_interface_list)
+        abbreviated_interface_list = INTERFACE_LIST_ORDERING_OPTIONS.get(order)(abbreviated_interface_list)  # type: ignore


Resolvable through explicit optional handling

Kircheneer · 2022-06-27T08:25:06Z

netutils/interface.py

@@ -556,7 +575,7 @@ def interface_range_compress(interface_list: t.List[str]) -> t.List[str]:
    Returns:
        list: list of interface ranges
    """
-    result_dict = {}
+    result_dict = {}  # type: ignore


result_dict: t.Dict[str, t.List[int]] = {}

Kircheneer · 2022-06-27T08:27:57Z

netutils/ip.py

@@ -174,13 +176,13 @@ def is_ip(ip):
        >>>
    """
    try:
-        ip = ipaddress.ip_address(ip)
+        ip = ipaddress.ip_address(ip)  # type: ignore


Just doing ipaddress.ip_address(ip) here should work to get rid of the comment.

Kircheneer · 2022-06-27T08:31:24Z

netutils/ip.py

@@ -221,11 +223,11 @@ def netmask_to_cidr(netmask):
        23
    """
    if is_netmask(netmask):
-        return bin(int(ipaddress.ip_address(netmask))).count("1")
+        return bin(int(ipaddress.ip_address(netmask))).count("1")  # type: ignore


Should change the return value of the function to int, then we can get rid of the ignore comment.

Kircheneer · 2022-06-27T08:32:29Z

netutils/ip.py

@@ -269,7 +271,7 @@ def cidr_to_netmaskv6(cidr):
    raise ValueError("Parameter must be an integer between 0 and 128.")


-def get_all_host(ip_network):
+def get_all_host(ip_network: str) -> t.List[str]:


-> Generator[str, None, None] and then we can also get rid of the ignore comment

Kircheneer · 2022-06-27T08:34:11Z

netutils/ip.py

@@ -326,7 +328,7 @@ def get_first_usable(ip_network):
    return str(net[1])


-def get_peer_ip(ip_interface):
+def get_peer_ip(ip_interface: str) -> t.Any:


Should be able to just return str here

Kircheneer · 2022-06-27T08:44:40Z

netutils/lib_mapper.py


-_NETMIKO_LIB_MAPPER = {
+_NETMIKO_LIB_MAPPER: t.Dict[str, t.Dict[str, str]] = {


This could be another place to use the Enum for network OSes

Kircheneer · 2022-06-27T08:49:30Z

netutils/mac.py

-def _valid_mac(func):
+def _valid_mac(func):  # type: ignore
    """Decorator to validate a MAC address is valid."""

    @wraps(func)
-    def decorated(*args, **kwargs):
+    def decorated(*args, **kwargs):  # type: ignore
        if kwargs.get("mac"):
            mac = kwargs.get("mac")
        else:
            mac = args[0]
-        if not is_valid_mac(mac):
+        if not is_valid_mac(mac):  # type: ignore
            raise ValueError(f"There was not a valid mac address in: `{mac}`")
        return func(*args, **kwargs)

    return decorated


Could still be more precise, but this should be enough for this:

"""Decorator to validate a MAC address is valid.""" @wraps(func) def decorated(*args: t.Any, **kwargs: t.Any) -> t.Any: if kwargs.get("mac"): mac = kwargs.get("mac") else: mac = args[0] assert isinstance(mac, str) if not is_valid_mac(mac): raise ValueError(f"There was not a valid mac address in: `{mac}`") return func(*args, **kwargs) return decorated

Kircheneer · 2022-06-27T08:49:56Z

netutils/mac.py

@@ -44,8 +45,8 @@ def is_valid_mac(mac):
    return False


-@_valid_mac
-def mac_to_format(mac, frmt="MAC_NO_SPECIAL"):
+@_valid_mac  # type: ignore


Can get rid of all of these once the decorator is fixed

Kircheneer · 2022-06-27T08:52:02Z

netutils/password.py

@@ -103,12 +106,14 @@ def compare_type5(unencrypted_password, encrypted_password, return_original=Fals
    salt = get_hash_salt(encrypted_password)
    if encrypt_type5(unencrypted_password, salt) == encrypted_password:
        if return_original is True:
-            return encrypted_password
+            return encrypted_password  # type: ignore


Return value of the function needs to be Union[str, bool], then we can get rid of this (but need to probably perform explicit handling of that return type in lots of places).

Kircheneer · 2022-06-27T08:52:55Z

netutils/password.py

    if not salt:
-        salt = random.randrange(0, 15)  # nosec
-    encrypted_password = "%02x" % salt  # pylint: disable=consider-using-f-string
+        salt = random.randrange(0, 15)  # type: ignore # nosec
+    encrypted_password = "%02x" % salt  # type: ignore # pylint: disable=consider-using-f-string
    for i, _ in enumerate(unencrypted_password):
-        hex_password = "%02x" % (ord(unencrypted_password[i]) ^ XLAT[salt])  # pylint: disable=consider-using-f-string
+        hex_password = "%02x" % (ord(unencrypted_password[i]) ^ XLAT[salt])  # type: ignore # pylint: disable=consider-using-f-string
        encrypted_password += hex_password
-        salt += 1
-        if salt == 51:
-            salt = 0
+        salt += 1  # type: ignore
+        if salt == 51:  # type: ignore
+            salt = 0  # type: ignore


If we use a different variable name for the changed form of salt, we can get by without all the ignore comments

Kircheneer · 2022-06-27T08:53:55Z

netutils/ping.py



-def tcp_ping(ip, port, timeout=1):  # pylint: disable=invalid-name
+def tcp_ping(ip: str, port: int, timeout: t.Optional[int] = 1) -> bool:  # pylint: disable=invalid-name


Can timeout be None in any case? If not, we can also get rid of the below ignore comments

Kircheneer · 2022-06-27T08:59:02Z

netutils/vlan.py

+    first_line_len: t.Optional[int] = 48,
+    other_line_len: t.Optional[int] = 44,
+    min_grouping_size: t.Optional[int] = 3,


All of these probably shouldn't be optional, then we can also get rid of some of the ignores below

jeffkala · 2022-06-28T02:46:59Z

@Kircheneer thanks so much for the thorough review, the detail is incredibly helpful. I'll work through getting everything updated and retested.

jeffkala · 2022-06-30T14:22:53Z

This will be closed, all these changes are also in #125

itdependsnetworks · 2022-07-15T21:04:21Z

Closing in favor of #125

jeffkala added 2 commits June 23, 2022 15:53

initial pass at type hinting and getting mypy to run

b6d57d8

update tasks and ci to do mypy as well

a00dcd9

jeffkala requested review from itdependsnetworks and qduk as code owners June 24, 2022 13:56

jeffkala added 3 commits June 24, 2022 09:00

rm unused import

7e0b924

fix contributing end-line number

6327e13

fix contributing end-line number

eba1a0a

Kircheneer reviewed Jun 27, 2022

View reviewed changes

This was referenced Jun 29, 2022

Add type hints to the whole project and mypy testing setup and CI #125

Closed

Clarify None handling for configuration parsing #126

Open

jeffkala marked this pull request as draft June 30, 2022 14:23

itdependsnetworks closed this Jul 15, 2022

jeffkala deleted the jkala-mypy branch October 12, 2022 22:03



		def clean_config(config, filters):
		def clean_config(config: str, filters: t.List[str]) -> str:

		backup_str = section_config(feature, backup_cfg, network_os) # type: ignore
		intended_str = section_config(feature, intended_cfg, network_os) # type: ignore

		comment_chars: t.List[str] = ["!"]
		banner_start: t.List[str] = ["banner", "vacant-message"]


		_NETMIKO_LIB_MAPPER = {
		_NETMIKO_LIB_MAPPER: t.Dict[str, t.Dict[str, str]] = {



		def tcp_ping(ip, port, timeout=1): # pylint: disable=invalid-name
		def tcp_ping(ip: str, port: int, timeout: t.Optional[int] = 1) -> bool: # pylint: disable=invalid-name

Add type hints to the whole project and mypy testing setup and CI #123

Add type hints to the whole project and mypy testing setup and CI #123

Conversation

jeffkala commented Jun 24, 2022

itdependsnetworks commented Jun 26, 2022

Kircheneer commented Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

Kircheneer Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kircheneer Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kircheneer Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffkala commented Jun 28, 2022

jeffkala commented Jun 30, 2022

itdependsnetworks commented Jul 15, 2022

Kircheneer commented Jun 27, 2022 •

edited

Loading

Kircheneer Jun 27, 2022 •

edited

Loading

Kircheneer Jun 27, 2022 •

edited

Loading

Kircheneer Jun 27, 2022 •

edited

Loading