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

Add list cmd #80

Closed
wants to merge 6 commits into from
Closed

Add list cmd #80

wants to merge 6 commits into from

Conversation

triole
Copy link

@triole triole commented Dec 3, 2023

Hi there,

thanks for this useful little tool. I thought it would be nice to just have a simple list command that prints a list of the addresses in a given cidr range. I am not sure if you find it useful as well but if so, please feel free to merge this pull request.

Cheers.

@bschaatsbergen
Copy link
Owner

Hello @triole, I appreciate your effort in submitting this PR, and your kind words mean a lot. It's great to hear that you find cidr useful! I have a few comments on the pull request and would like to discuss them with you.

  • The intention behind cidr is to be agnostic to both IPv4 and IPv6, offering features that naturally support both, as per the existing CLI features.
  • While this feature is beneficial, I have concerns about its practicality for CIDR ranges larger than /24. Printing thousands of lines to the console quickly becomes overwhelming. What are your thoughts on this?
  • Regarding the implementation, the current approach involves building the list in-memory, which might not be the most efficient solution.

Let's discuss and brainstorm on these points.

@bschaatsbergen bschaatsbergen added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 4, 2023
@triole
Copy link
Author

triole commented Dec 5, 2023

Hi,

thanks for your ideas and comments. I am glad that we can discuss here to help each other understand a little better. Let me try to explain why I find this feature useful.

But first let me say that regarding the implementation you are absolutely right. It wasn't a good idea to store the addresses inside a list and print them later. Something that I also noticed a little later but at this point the pull request was already made. I guess sometimes it's better to sleep over things. Anyhow I updated this and hope that it is solved better now.

Concerning your other point that an address list of bigger cidr ranges can grow quite large I must admit that I do not see this as a big problem. I suppose the most use cases are just checking smaller ranges and have a look at which addresses belong to these. But of course you could also try larger ones. From my point of view this is a feature that I would not decide to keep back from users just because it might end up in a print orgy that by the way can always be aborted pressing ctrl+c.. I would leave the decision about the usefulness of the list feature to the users. If someone wants to have a look at thousands of ip addresses, why not? Furthermore there might be occasions where people just need these kind of large lists to process them further with shell tools like grep, tail, head or anything else. You could also think of piping lists into files and continue working with them from there.

So anyway the point is that I think I get what you mean, but I would not decide to not offer this feature. There might be people who have use cases for it. So let the user decide what to do with it.

I hope the above makes senses and helps a little.

Cheers.

@bschaatsbergen bschaatsbergen removed the waiting-response Maintainers are waiting on response from community or contributor. label Dec 9, 2023
@bschaatsbergen
Copy link
Owner

bschaatsbergen commented Dec 9, 2023

Hi @triole,

It's great to hear that you've revisited the implementation and made updates to address the concerns regarding storing addresses in a list. Taking a step back and reconsidering the approach can indeed lead to more robust solutions.

My primary concern is around the scalability of the feature. As CIDR ranges with small prefix lengths can indeed be more manageable, the situation might become less favorable as the prefix length increases. Handling thousands or even millions of IP addresses could lead to performance issues and might not align with the intended user experience.

Considering these factors, I wonder if there could be a middle ground that allows users to explore smaller CIDR ranges more interactively while also providing warnings or limitations for larger ranges. This approach could strike a balance between user experience and cidr's efficiency.

I'm not fully convinced yet of the ideal solution, and I believe it's a complex matter that requires careful consideration. It's great that you're open to feedback @triole , and I look forward to seeing how this feature develops - what are your thoughts on this @DanielRieske, @mmourick and @mvanholsteijn?

@DanielRieske
Copy link
Collaborator

Hi 👋 @bschaatsbergen @triole

Aside from scalability concerns that I absolutely agree with, I do have some questions regarding usability.
I am wondering how useful this feature would be for larger CIDR ranges, while I won't dispute that it has its merits for smaller CIDR ranges I would look into options to make more of a summary of a given CIDR range.

Example, given CIDR range 172.16.0.0/24 you can print out a summary with the following details

IP Address Network Address Usable Host IP Range Total Number of Hosts
172.16.0.0/24 172.16.0.0 172.16.0.1 - 172.16.0.254 256

This approach can also be used to calculate IPV6 addresses, therefore we keep supporting both.

Curious what you guys think of this approach

@bschaatsbergen
Copy link
Owner

bschaatsbergen commented Dec 9, 2023

Hi 👋 @bschaatsbergen @triole

Aside from scalability concerns that I absolutely agree with, I do have some questions regarding usability. I am wondering how useful this feature would be for larger CIDR ranges, while I won't dispute that it has its merits for smaller CIDR ranges I would look into options to make more of a summary of a given CIDR range.

Example, given CIDR range 172.16.0.0/24 you can print out a summary with the following details

IP Address Network Address Usable Host IP Range Total Number of Hosts
172.16.0.0/24 172.16.0.0 172.16.0.1 - 172.16.0.254 256
This approach can also be used to calculate IPV6 addresses, therefore we keep supporting both.

Curious what you guys think of this approach

Appreciate you sharing your view on this, @DanielRieske! Your input aligns with my current thinking, and I'm actively incorporating it into #75 as I'm typing this.

image

@bschaatsbergen bschaatsbergen added under-consideration Indicates that the topic or suggestion is actively being reviewed and considered for implementation. waiting-response Maintainers are waiting on response from community or contributor. labels Dec 9, 2023
@triole
Copy link
Author

triole commented Dec 12, 2023

Hi @bschaatsbergen and @DanielRieske ,

I appreciate your feedback and I totally see your point regarding the scalability of the list feature. Nonetheless I do tend to leave the question of usability up to each individual who considers using the feature. Maybe a warning before printing too many addresses would be a reasonable middle ground that we all could settle with. Although of course immediately the question arises, how many actually are too many.

The explain feature definitely has its use and I'm excitedly looking forward to see it implemented. I think it'll be very handy but list is a different thing on purpose. Sometimes I just need stupid plain lists of IP addresses to be generated. As said I wouldn't limit the possibilities of list but of course I can live with the fact that you see this differently.

Thanks for the constructive discussion. I really enjoy it.

Cheers.

@bschaatsbergen
Copy link
Owner

bschaatsbergen commented Jan 30, 2024

Hi @bschaatsbergen and @DanielRieske ,

I appreciate your feedback and I totally see your point regarding the scalability of the list feature. Nonetheless I do tend to leave the question of usability up to each individual who considers using the feature. Maybe a warning before printing too many addresses would be a reasonable middle ground that we all could settle with. Although of course immediately the question arises, how many actually are too many.

The explain feature definitely has its use and I'm excitedly looking forward to see it implemented. I think it'll be very handy but list is a different thing on purpose. Sometimes I just need stupid plain lists of IP addresses to be generated. As said I wouldn't limit the possibilities of list but of course I can live with the fact that you see this differently.

Thanks for the constructive discussion. I really enjoy it.

Cheers.

Hey @triole,

I've been doing some tinkering about this and please allow me to get back on this in a couple days :)
Meawhile, what's your view on the explain subcommand we've added?

@triole
Copy link
Author

triole commented Feb 14, 2024

Hi, no worries. It is absolutely fine not to answer or expect an answer on the same day. I know how it is to be busy all the time and therefore being forced to postpone things over and over again. It's all fine.

But to answer your question: I like the explain command and find it quite useful to get some information about network ranges, netmask, the broadcast address and so on. As I said earlier this is a nice feature that definitely has its purpose. Nonetheless the use case is different from the list feature because if you need a stupid plain list of IP addresses to further process them somewhere else list would be the way to go.

I guess you know my standpoint. I'd like to see both features implemented. But I totally respect if you decide otherwise.

Cheers.

@bschaatsbergen
Copy link
Owner

Thank you for your understanding, and we appreciate your input on the explain command and list feature. At this time, we're not planning to implement the list feature, but your feedback is valuable. If there's increased community support or demand for this particular feature, we'll certainly reconsider. @triole

@bschaatsbergen bschaatsbergen removed waiting-response Maintainers are waiting on response from community or contributor. under-consideration Indicates that the topic or suggestion is actively being reviewed and considered for implementation. labels Feb 15, 2024
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