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

feature: add divide subcommand #100

Merged
merged 23 commits into from
Jun 29, 2024

Conversation

Phaze228
Copy link
Contributor

@Phaze228 Phaze228 commented May 23, 2024

What

  • fixed an overflow bug, using big.Ints
  • added a new subcommand: divide

Closes #89

@github-actions github-actions bot added the tests Pull requestss that update or introduce tests label May 23, 2024
@bschaatsbergen
Copy link
Owner

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! 👏🏼

cmd/divide.go Fixed Show fixed Hide fixed
cmd/next.go Fixed Show fixed Hide fixed
cmd/next.go Fixed Show fixed Hide fixed
@Phaze228
Copy link
Contributor Author

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.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label May 25, 2024
@Phaze228
Copy link
Contributor Author

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

@bschaatsbergen
Copy link
Owner

bschaatsbergen commented May 28, 2024

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
Copy link
Contributor Author

Beautiful. Thank you.

@Phaze228 Phaze228 closed this May 29, 2024
@Phaze228 Phaze228 reopened this May 29, 2024
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
cmd/count.go Show resolved Hide resolved
cmd/divide.go Outdated Show resolved Hide resolved
cmd/explain.go Outdated Show resolved Hide resolved
cmd/divide.go Outdated Show resolved Hide resolved
cmd/next.go Outdated Show resolved Hide resolved
Phaze228 and others added 2 commits May 29, 2024 21:50
Textual changes.

Co-authored-by: Bruno Schaatsbergen <[email protected]>
…formatter | formatter: Added simple number formatter
cmd/divide.go Outdated Show resolved Hide resolved
@bschaatsbergen
Copy link
Owner

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
Copy link
Owner

@mmourick requesting your review too 👍🏼

Copy link
Collaborator

@mmourick mmourick left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! This looks like a great addition to cidr!

I've left a few small nitpicks, but besides that I think it's almost good to go. Just need to run gofmt as currently golangci-lint is failing.

pkg/core/core.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
cmd/explain.go Outdated Show resolved Hide resolved
@Phaze228
Copy link
Contributor Author

Phaze228 commented Jun 7, 2024

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
Copy link
Owner

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

@Phaze228
Copy link
Contributor Author

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

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Jun 29, 2024
@bschaatsbergen bschaatsbergen changed the title Fixed IPv6 address overflow// Added next CIDR command // Added Divide… feature: add divide subcommand Jun 29, 2024
@bschaatsbergen
Copy link
Owner

bschaatsbergen commented Jun 29, 2024

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
bschaatsbergen previously approved these changes Jun 29, 2024
Copy link
Owner

@bschaatsbergen bschaatsbergen left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@bschaatsbergen
Copy link
Owner

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.

Copy link
Owner

@bschaatsbergen bschaatsbergen left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@bschaatsbergen bschaatsbergen merged commit eb8ca36 into bschaatsbergen:main Jun 29, 2024
4 checks passed
@bschaatsbergen bschaatsbergen deleted the add-next-divide branch June 29, 2024 16:43
@bschaatsbergen
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation minor new-feature New feature tests Pull requestss that update or introduce tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to divide a CIDR range into smaller ranges
3 participants