-
Notifications
You must be signed in to change notification settings - Fork 48
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.
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
Add type hints to the whole project and mypy testing setup and CI #123
Conversation
Pretty cool stuff!! Will ping @Kircheneer to ask for an extra pair of eyes. |
Brilliant stuff @jeffkala! I have a couple of meta-level comments and will dive into the down and dirty in a moment.
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:
|
if nbr_decimal == 0: | ||
nbr_decimal = None | ||
nbr_decimal = None # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that round(x, None) == round(x)
for all x
, right? Meaning we could drop this branch and the corresponding # type: ignore
.
|
||
|
||
def clean_config(config, filters): | ||
def clean_config(config: str, filters: t.List[str]) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]]
).
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See last comment, can be removed if the function signature is fixed.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing as with clean_config
from . import parser # pylint: disable=relative-beyond-top-level | ||
|
||
parser_map = { | ||
parser_map: t.Dict[str, t.Callable] = { # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@@ -15,7 +17,7 @@ | |||
"nokia_sros": parser.NokiaConfigParser, | |||
} | |||
|
|||
default_feature = { | |||
default_feature: t.Dict[str, t.Union[str, bool, None]] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I see this correctly this would be another place we could use the proposed default_feature
TypedDict
.
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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?
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]]]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another possible place to use the default_feature
TypedDict
?
backup: str, | ||
intended: str, | ||
network_os: str, | ||
cfg_type: t.Optional[str] = "file", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
backup_str = section_config(feature, backup_cfg, network_os) # type: ignore | ||
intended_str = section_config(feature, intended_cfg, network_os) # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be fixed by changing the return type of open_file_config
to just a str
.
|
||
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible fix to build the feature
TypedDict
out and use it as the return value?
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be fixed with the feature
TypedDict
as feature.get("section")
would return something whose type Mypy would be able to infer correctly.
comment_chars: t.List[str] = ["!"] | ||
banner_start: t.List[str] = ["banner", "vacant-message"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
comment_chars: t.List[str] = ["!"] | ||
banner_start: t.List[str] = ["banner", "vacant-message"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can also get rid of these annotations.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be fixed by the proposed changes to the base class.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use t.Optional[str]
here to simplify the type a little
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by proposed changes to the base class.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolvable by explicit optional handling further up.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add this issue to this comment: python/mypy#5374
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should only look at these once the bug with Mypy is addressed.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be fixed with def __lt__(self, other: "CharacterClass") -> bool: # noqa: D105
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be fixed with def __lt__(self, other: "CharacterClass") -> bool: # noqa: D105
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be fixed with def __lt__(self, other: "CharacterClass") -> bool: # noqa: D105
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should return NoReturn
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this ever need the None
value, i.e. does it make a difference if its just False
?
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolvable through explicit optional handling
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result_dict: t.Dict[str, t.List[int]] = {}
@@ -174,13 +176,13 @@ def is_ip(ip): | |||
>>> | |||
""" | |||
try: | |||
ip = ipaddress.ip_address(ip) | |||
ip = ipaddress.ip_address(ip) # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just doing ipaddress.ip_address(ip)
here should work to get rid of the comment.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should change the return value of the function to int
, then we can get rid of the ignore comment.
@@ -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]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> Generator[str, None, None]
and then we can also get rid of the ignore comment
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to just return str
here
|
||
_NETMIKO_LIB_MAPPER = { | ||
_NETMIKO_LIB_MAPPER: t.Dict[str, t.Dict[str, str]] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be another place to use the Enum for network OSes
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can get rid of all of these once the decorator is fixed
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use a different variable name for the changed form of salt
, we can get by without all the ignore comments
|
||
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can timeout
be None
in any case? If not, we can also get rid of the below ignore comments
first_line_len: t.Optional[int] = 48, | ||
other_line_len: t.Optional[int] = 44, | ||
min_grouping_size: t.Optional[int] = 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these probably shouldn't be optional, then we can also get rid of some of the ignores below
@Kircheneer thanks so much for the thorough review, the detail is incredibly helpful. I'll work through getting everything updated and retested. |
This will be closed, all these changes are also in #125 |
Closing in favor of #125 |
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.