Skip to content
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

Monkeytype Juniper family. #254

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Monkeytype Juniper family. #254

wants to merge 5 commits into from

Conversation

ankenyr
Copy link
Collaborator

@ankenyr ankenyr commented Feb 20, 2023

No description provided.

@ankenyr ankenyr requested a review from jtwb February 20, 2023 04:47
aerleon/lib/juniper.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 20, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change

Comparison is base (b6242d0) 91.08% compared to head (6746c34) 91.09%.

❗ Current head 6746c34 differs from pull request most recent head 4c1ac44. Consider uploading reports for the commit 4c1ac44 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #254   +/-   ##
=======================================
  Coverage   91.08%   91.09%           
=======================================
  Files          98       98           
  Lines       22058    22076   +18     
  Branches     3315     3315           
=======================================
+ Hits        20092    20110   +18     
  Misses       1279     1279           
  Partials      687      687           
Flag Coverage Δ
tests 91.07% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aerleon/lib/juniper.py 86.90% <100.00%> (+0.10%) ⬆️
aerleon/lib/juniperevo.py 98.03% <100.00%> (ø)
aerleon/lib/junipermsmpc.py 90.02% <100.00%> (+0.05%) ⬆️
aerleon/lib/junipersrx.py 90.14% <100.00%> (+0.03%) ⬆️
aerleon/lib/ciscoxr.py 100.00% <0.00%> (ø)
aerleon/lib/cisco.py 88.70% <0.00%> (+0.07%) ⬆️
aerleon/lib/ciscoasa.py 57.64% <0.00%> (+0.50%) ⬆️
aerleon/lib/arista.py 74.07% <0.00%> (+0.99%) ⬆️
aerleon/lib/cisconx.py 74.07% <0.00%> (+0.99%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ankenyr ankenyr requested a review from jtwb February 26, 2023 19:04
def _Comment(self, addr, exclude=False, line_length=132):
def _Comment(
self, addr: Union[IPv6, IPv4, DSMNet], exclude: bool = False, line_length: int = 132
) -> List[Union[Any, str]]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Looking at the code, I think the return type is List[str].
  2. The docstring for argument addr is out of date.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -848,7 +856,7 @@ def _Comment(self, addr, exclude=False, line_length=132):
logging.warning('Ignoring non IPv4 or IPv6 address: %s', addr)
return rval

def _Group(self, group, lc=True):
def _Group(self, group: List[Union[int, Tuple[int, int], str]], lc: bool = True) -> str:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily in scope, but just spotted this duplicated expression on line 883:

if isinstance(el, str) or isinstance(el, str):

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -416,7 +419,7 @@ def _BuildPort(self, ports):
port_list.append('%s-%s' % (str(p[0]), str(p[1])))
return port_list

def _GenerateApplications(self, filter_name):
def _GenerateApplications(self, filter_name: str) -> List[Union[Any, str]]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type is List[str]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -399,7 +402,7 @@ def _BuildTokens(self):
)
return supported_tokens, supported_sub_tokens

def _BuildPort(self, ports):
def _BuildPort(self, ports: List[Union[Any, List[int], Tuple[int, int]]]) -> List[str]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Monkeytype seems to have noticed this function is called with lists instead of tuples sometimes, and sure enough on line 616 (pre-changes), _TranslatePolicy constructs a list instead of a tuple to represent a port range. I'd suggest clamping the type to List[Tuple[int, int]] and then cleaning up line 616 to use a tuple.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -613,7 +615,7 @@ def _TranslatePolicy(self, pol, exp_info):

self.srx_policies.append((header, new_terms, filter_options))

def _FixLargePolices(self, terms, address_family):
def _FixLargePolices(self, terms: List[Any], address_family: str) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"terms" is List[policy.Term].

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -675,7 +677,7 @@ def Chunks(addresses):
del terms[:]
terms.extend(expanded_terms)

def _BuildPort(self, ports):
def _BuildPort(self, ports: List[Union[Any, Tuple[int, int]]]) -> List[Union[Any, str]]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both "Any" hints can be removed. The type for "ports" is List[Tuple[int, int]] and the return type is List[str]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@ankenyr ankenyr requested a review from jtwb March 4, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants