search

bschaatsbergen / cidr

Simplifies IPv4/IPv6 CIDR network prefix management with counting, overlap checking, explanation, and subdivision.
https://formulae.brew.sh/formula/cidr
MIT License
168 stars 9 forks source link

feature: add `divide` subcommand #100

Closed Phaze228 closed 1 month ago

Phaze228 commented 3 months ago

What

Closes #89

bschaatsbergen commented 3 months ago

Hey @Phaze228, thank you very much for submitting this pull request. Give me a couple working days (I'll be out over the weekend) to review this. Your contribution is much appreciated! 👏🏼

Phaze228 commented 2 months ago

Not a problem, I may clean up a few things, over the weekend. Notably, missing the core_test.go file was testing the AddressCount function, and a few other little things the auto-analyzer is yelling at. But functionally, it seems good. Probably have to come up with a nice way to handle dividing ipv6 spaces, because if you want to do something stupid like dividing a 2001::0000/4 into 90,000,000 different spaces, that all gets printed to the console.

Phaze228 commented 2 months ago

I will say, it's a little rude of the linter to point out my mistakes without files and line numbers.

bschaatsbergen commented 2 months ago

I will say, it's a little rude of the linter to point out my mistakes without files and line numbers.

I've updated your branch to include 7d286eb. In v6.0.1 of the golangci-lint-action they stopped setting the output format to github-actions and it now defaults to colored-line-number which fixes the problem of not being able to see line numbers and file names. Comments are now also applied to the PR by the action.

Phaze228 commented 2 months ago

Beautiful. Thank you.

bschaatsbergen commented 2 months ago

I'm very excited to get this merged, thanks for your hard work. It's just a few minor tweaks we need to make.

bschaatsbergen commented 2 months ago

@mmourick requesting your review too 👍🏼

Phaze228 commented 2 months ago

Took in everyone's suggestions. Minus removing the shortened -H for hosts flag. My preference is for it, but you guys can adjust it if you want before merging. Hopefully the linter doesn't continue to hate me.

bschaatsbergen commented 2 months ago

Hey @Phaze228, update: I'm working locally on your branch 👍🏼

Phaze228 commented 2 months ago

Alrighty, sounds good. Let me know if you need anything.

bschaatsbergen commented 1 month ago

Hey @Phaze228, apologies for the slower review cycle. Work has been a bit hectic lately. I trimmed down this pull request to just include the divide command to make the review process easier. I’m very keen on getting the next subcommand in, but I’d like to review and submit that separately through a follow-up PR.

Could you please elaborate on one thing? I’m curious about why we introduced a format function and what issues were found with the existing formatting. The previous implementation seemed less complex and didn’t have any issues, as far as I can recall.

bschaatsbergen commented 1 month ago

Could you please elaborate on one thing? I’m curious about why we introduced a format function and what issues were found with the existing formatting. The previous implementation seemed less complex and didn’t have any issues, as far as I can recall.

It appears that the NewPrinter from golang.org/x/text/language cannot handle a *big.Int.

bschaatsbergen commented 1 month ago

Thank you for your hard work on this, @Phaze228! It’s greatly appreciated 👏🏼!