Skip to content

Conversation

@Kashaf-Sajid-Yaseen
Copy link
Contributor

@Kashaf-Sajid-Yaseen Kashaf-Sajid-Yaseen commented Aug 21, 2023

Tools to check the type of input (IPv4 & IPv6) entered by the user.

@awaismslm
Copy link
Contributor

@UmanShahzad Please check.

Copy link
Contributor

@UmanShahzad UmanShahzad left a comment

Choose a reason for hiding this comment

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

Do fix issues commonly found in the other PRs that also appear here, then do ping me again @Kashaf-Sajid-Yaseen @awaismslm

@Kashaf-Sajid-Yaseen
Copy link
Contributor Author

@UmanShahzad Please check.

Copy link
Contributor

@UmanShahzad UmanShahzad left a comment

Choose a reason for hiding this comment

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

Few things:

  • Ensure we dont use underscores in the name of Golang functions. In the subcommand name itself it's fine but not in golang code.
  • For printing, we should just print <ip>,true or <ip>,false for now instead of the full sentences of <ip> is na IPv4 input
  • Can we develop helper functions to check if the IP or IPNet (cidr) is v4/v6? Too many inlined conditionals are being used which makes it harder to read and reuse.

case cmd == "is_v4":
err = cmdToolIs_v4()
case cmd == "is_v6":
err = cmdToolIs_v6()
Copy link
Contributor

Choose a reason for hiding this comment

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

Func name should be cmdToolIsV6 (Similar for v4), even though the actual subcmd has an underscore.

Examples:
# Check CIDR.
$ %[1]s tool is_v6 1.1.1.0/30
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we give a real v6 example

@Kashaf-Sajid-Yaseen
Copy link
Contributor Author

@UmanShahzad Please check.

@UmanShahzad UmanShahzad merged commit fb3abc4 into ipinfo:master Aug 23, 2023
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.

3 participants