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

Provide add/subtract methods for arithmetic calculations on addresses #51

Closed

Conversation

victorhahncastell
Copy link

Unit tests provided and passing :-)

@bluemonk
Copy link
Collaborator

bluemonk commented Feb 2, 2015

That's an impressive PR Victor, thanks a lot.

It will take me some time to go through it so bear with me for the moment please. At a first sight, I like the concept but i find the UI a bit confusing, maybe we can think of something better.

@victorhahncastell
Copy link
Author

Sure, feel free to just change it or drop me any suggestions :-)

@victorhahncastell
Copy link
Author

Hi Marco,

are you still planning to take this feature upstream? Is there anything I can adjust to have this better match your code?

As I currently work on a project using this feature I would love to see this in the main branch.

Best,
Victor

@lumean
Copy link

lumean commented Mar 22, 2016

Hi, I'd also like to use this for a project. Currently i just monkeypatch the most needed methods into the Ipaddress classes, but having this upstream would be much more convenient.
br manuel

@francisluong
Copy link
Contributor

Having a look. @victorhahncastell: if you're still plugged in can you re-merge master and resolve conflicts?

@@ -13,13 +13,13 @@ def setup
"10.0.0.1/255.255.255.0" => ["10.0.0.1", 24]}

@invalid_ipv4 = ["10.0.0.256",
"10.0.0.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

why the indent level changes?

@@ -433,6 +433,57 @@ def test_method_supernet
assert_equal "172.16.8.0/22", @ip.supernet(22).to_string
end

def test_method_add
ip = IPAddress::IPv4.new("172.16.10.1/24")
assert_equal ip.add(5), IPAddress::IPv4.new("172.16.10.6/24")
Copy link
Contributor

Choose a reason for hiding this comment

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

For asserts, please add parens, per https://github.com/bbatsov/ruby-style-guide#no-dsl-parens

Copy link

Choose a reason for hiding this comment

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

There are no parens in the existing tests, should they be added everywhere? I'm currently integrating your suggestions.

@lumean
Copy link

lumean commented Apr 18, 2016

Hi @francisluong , I would very much appreciate if we can continue this in #79 and then close them both. I hope this is ok with @victorhahncastell since he didn't give any feedback so far.

@victorhahncastell
Copy link
Author

@lumean: Sure, thanks for handling this :)

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.

4 participants