-
Notifications
You must be signed in to change notification settings - Fork 17
WIP: Refactor to add ability to use it as a lib #10
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
base: master
Are you sure you want to change the base?
Conversation
exit(1) | ||
header = [] | ||
header.append("IX") | ||
def find_common_points(common_ix_list: List, common_fac_list: List, peers: List) -> Dict: |
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.
Consider a return type of List[IXP] for this method
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 will refactor this and split it in two functions which can be called independ.
v4 = "v4: " + "\n".join([str(i) for i in curr_ix.subnet4]) | ||
v6 = "v6: " + "\n".join( | ||
[str(i) for i in curr_ix.subnet6 if str(i) != "0100::"] | ||
curr_ix = asdict(fetch_ix_from_ixps(ix, peer.peering_on)) |
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.
Please avoid rednering the dataclass to dict, rather keep track of common IXPs by appending the IXP dataclass to a list, and returning that.
Move the logic for the rendering for the ipv4/ipv6 to human format to human_readable_print, and use subnet4/subnet6 to capture the list of IP Addresses in the IXP dataclass.
curr_ix = fetch_ix_from_ixps(ix, peer.peering_on) | ||
v4 = "v4: " + "\n".join([str(i) for i in curr_ix.subnet4]) | ||
v6 = "v6: " + "\n".join( | ||
[str(i) for i in curr_ix.subnet6 if str(i) != "0100::"] |
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 use the IPv6 prefix 0100:: to denote that this peer does not have a v6 address. So please ensure you capture this logic here.
args = getArgs() | ||
peers = get_peers(args) | ||
|
||
if args.ix_only and args.fac_only: |
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.
and args.fac_only is not needed here, as we will fallthrough to the next elif and print the required output.
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.
Please see inline comments.
No description provided.