-
Notifications
You must be signed in to change notification settings - Fork 157
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
IP calculator #154
IP calculator #154
Conversation
This commit is a part of Issue #108, which involves implementing the IP calculator. Specifically, it introduces the 'ip2n' command to convert IP addresses to their corresponding integers. The 'ip2n' subcommand now supports both IPv4 and IPv6 addresses. Usage: 'my_command ip2n <ip>' Examples: - 'ipinfo ip2n "190.87.89.1"' - 'ipinfo ip2n "2001:0db8:85a3:0000:0000:8a2e:0370:7334"' - 'ipinfo ip2n "2001:0db8:85a3::8a2e:0370:7334"' - 'ipinfo ip2n "::7334"' - 'ipinfo ip2n "7334::"' Fixes: #108
…IP calculator. Specifically, it introduces the 'n2ip' command to convert integers to their corresponding IP addresses. The 'n2ip' command supports both IPv4 and IPv6 addresses. Usage: 'ipinfo n2ip [<opts>] <expr>' Examples: 'ipinfo n2ip "2*2828-1"' 'ipinfo n2ip "190.87.89.1*2"' 'ipinfo n2ip "2001:0db8:85a3:0000:0000:8a2e:0370:7334*6"' Additionally, the '--ipv6' option, when specified, forces conversion to IPv6 address even if the result is within the IPv4 range. This implementation includes helper functions to validate expressions and convert integers to IP addresses. Fixes: #108
…IP calculator. Specifically, it introduces the 'n2ip6' command to convert decimal numbers to IPv6 addresses. Usage: 'ipinfo n2ip6 [<opts>] <expr>' Examples: 'ipinfo n2ip6 "12*121"' 'ipinfo n2ip6 "::2*2' 'ipinfo n2ip6 "192.132.12.32*2"' 'ipinfo n2ip6 "::2+4::"' Fixes: #108
…IP calculator. Specifically, it introduces the 'calc' command to perform arithmetic on IP addresses and numbers Usage: 'ipinfo calc <expr>' Examples: 'ipinfo calc "12*121-(2::*4)"' 'ipinfo calc "::2*2' 'ipinfo calc "192.132.12.32*2"' 'ipinfo calc "::2+4::"' Fixes: #108
…IP calculator. Specifically, it adds entries for 'calc', 'ip2n', 'n2ip', and 'n2ip6' commands in the default help message
…IP calculator. Specifically, it adds entries for 'calc', 'ip2n', 'n2ip', and 'n2ip6' commands in the completions.
…IP calculator. Specifically, it adds cases for 'calc', 'ip2n', 'n2ip', and 'n2ip6' commands in the switch block.
Co-authored-by: awaismslm <[email protected]>
Co-authored-by: awaismslm <[email protected]>
Co-authored-by: awaismslm <[email protected]>
Co-authored-by: awaismslm <[email protected]>
Co-authored-by: awaismslm <[email protected]>
Co-authored-by: awaismslm <[email protected]>
Co-authored-by: awaismslm <[email protected]>
- Added comments
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 are almost done. Just a few changes
- Removed unnecessary arguments check in handler functions - Removed whitespaces
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.
Some minor things. Please do the changes.
Co-authored-by: awaismslm <[email protected]>
Co-authored-by: awaismslm <[email protected]>
Co-authored-by: awaismslm <[email protected]>
Co-authored-by: awaismslm <[email protected]>
Co-authored-by: awaismslm <[email protected]>
Co-authored-by: awaismslm <[email protected]>
Co-authored-by: awaismslm <[email protected]>
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 a minor fix.
@UmanShahzad Please do the final review. I am done from my side. |
Co-authored-by: awaismslm <[email protected]>
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 can remove the
--nocolor
flag if color is not relevant to these commands. - My fault on this but, we should really move
ip2n
,n2ip
,n2ip6
intoipinfo tool
. These are not "big" enough or commonly used enough CLI use cases to warrant being at the root level.calc
is fine at the root for now. - I'll do another round of review soon and actually test it out soon but, does the expression evaluator allow for space-like characters in it? Ideally I can write
ipinfo calc '2 * 2'
and not get an error.
ipinfo/cmd_calc.go
Outdated
fmt.Printf( | ||
`Usage: %s calc <expression> [<opts>] | ||
|
||
calc <expression> |
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.
calc <expression> | |
Description: |
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.
- Yes! the
--nocolor
flag is unnecessary and it is removed in the latest commit. - Refactor is done, now
ip2n
,n2ip
,n2ip6
are nested insideipinfo tool
. - The
calc
command is resilient to white-spaces in the input.
lib/stack.go
Outdated
} | ||
} | ||
|
||
// Top Return top element of stack. Return false if stack is empty. |
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.
// Top Return top element of stack. Return false if stack is empty. | |
// Top returns the top element of the stack. Returns empty string if stack is empty. |
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.
Note that this isn't exactly the best interface because an empty string could be a real element.
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.
Yes you are right! I solved this by returning a boolean alongside the empty string if stack is empty.
And as far as empty string being a real element is concerned. The way we are deciding if the stack is empty is by using length. So it solves that issue.
lib/stack.go
Outdated
} | ||
|
||
// Pop Remove top element of stack. Return false if stack is empty. | ||
func (st *Stack) Pop() 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.
This implementation pops without returning the popped element. Not really ideal...
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. Combined the functionality of Pop
and Top
into a one function Pop
.
* Centralized regex patterns for `ipv4` and `ipv6` addresses in one place at `regex.go` for better code organization and maintainability. * Improved documentation * Found and solved a bug in `calc` where 2(2) would not work as 2*(2) * Improved Implementation of `Pop` inside `stack.go` and better documentation * Applied other suggestions from the review.
ipinfo/cmd_tool_n2ip6.go
Outdated
and can also evaluate a mathematical expression for conversion. | ||
|
||
Examples: | ||
%[1]s n2ip "4294967295 + 87" |
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.
Wrong cmd name being used
lib/ip_str.go
Outdated
// StrIsIPv6Str checks if the given string is an IPv6 address | ||
func StrIsIPv6Str(expression string) bool { | ||
// Compile the regular expression | ||
ipV6Regex := regexp.MustCompile(ipV6RgxPattern) |
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.
May have had some misunderstanding on this - we wanna compile these globally just once. This will compile it over and over each time redundantly.
Is the issue that in the original grepip
spot it was using MustCompilePOSIX and that doesn't work for you in this use case?
lib/regex.go
Outdated
var ipV4RgxPattern string | ||
|
||
// ipV6RgxPattern is a global variable used to store a regular expression pattern for matching IPv4 addresses. | ||
var ipV6RgxPattern string |
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.
Is rgx
a common abbreviation? Usually regex
is enough
lib/regex.go
Outdated
// GetIpV4RgxPattern returns a regular expression pattern for matching IPv6 addresses. | ||
// The returned regular expression can be used to validate strings and determine whether they represent | ||
// valid IPv4 addresses. | ||
func GetIpV4RgxPattern() string { |
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.
You don't really need a function for it, you can just assign the string here directly to the global variable at definition above (and mark it const
too)
lib/init.go
Outdated
@@ -3,4 +3,6 @@ package lib | |||
func init() { | |||
bogonIP4List = GetBogonRange4() | |||
bogonIP6List = GetBogonRange6() | |||
ipV4RgxPattern = GetIpV4RgxPattern() | |||
ipV6RgxPattern = GetIpV6RgxPattern() |
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.
Won't be needed here as it's just a string, you can assign it directly
lib/cmd_grepip.go
Outdated
} else { | ||
rexp = regexp.MustCompilePOSIX(rexp4 + "|" + rexp6) | ||
rexp = regexp.MustCompilePOSIX(ipV4RgxPattern + "|" + ipV6RgxPattern) |
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.
No need to even compile it here if we compiled it before at the global level - we can just assign rexp
to be the 'correct' global regexp variable
lib/cmd_calc.go
Outdated
|
||
// CmdCalc Function is the handler for the "calc" command. | ||
func CmdCalc(f CmdCalcFlags, args []string, printHelp func()) error { | ||
if len(args) == 0 { |
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.
What about the ipinfo calc -h
case, is that being handled?
lib/cmd_tool_n2ip.go
Outdated
|
||
// CmdToolN2IP converts a number to an IP address | ||
func CmdToolN2IP(f CmdToolN2IPFlags, args []string, printHelp func()) error { | ||
if len(args) == 0 { |
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.
What about the ipinfo tool n2ip -h
case, is that being handled?
lib/cmd_tool_n2ip6.go
Outdated
|
||
// CmdToolN2IP6 converts a number to an IPv6 address | ||
func CmdToolN2IP6(args []string, printHelp func()) error { | ||
if len(args) == 0 { |
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.
What about the ipinfo tool n2ip6 -h
case, is that being handled?
Co-authored-by: Uman Shahzad <[email protected]>
- Fixed Help flags
Co-authored-by: Uman Shahzad <[email protected]>
Does not affect the validation of IPs in calc.
ipinfo calc <expr>
: This sub-command allows performing mathematical calculations on integers and IP addresses using infix expressions. It supports operators like (, ), ^, *, /, +, and -. The implementation involves converting IPs to numbers, converting infix expressions to postfix notation, and executing a stack-based algorithm to calculate the result.Examples:
ipinfo calc '2*(1.1.1.0 + 2)' ipinfo calc '::/2' ipinfo calc '2*2'
ipinfo ip2n
: This sub-command converts an IP address to its corresponding numeric representation.Examples
ipinfo ip2n 192.168.1.1 ipinfo ip2n 2001:0db8:85a3::8a2e:0370:7334
ipinfo n2ip
: This sub-command converts a numeric representation to an IP address. It also supports forcing IPv6 if necessary.Examples:
ipinfo n2ip -6 3232235777
ipinfo n2ip 32335777
ipinfo n2ip6
: This sub-command is a forced IPv6 version of the n2ip sub-command.Example:
ipinfo n2ip6 3232235777
Closes #108