-
Notifications
You must be signed in to change notification settings - Fork 97
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
Provide add/subtract methods for arithmetic calculations on addresses #51
Conversation
… instead of minitest for Ruby > 1.9.0
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. |
Sure, feel free to just change it or drop me any suggestions :-) |
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, |
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. |
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", |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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. |
@lumean: Sure, thanks for handling this :) |
Unit tests provided and passing :-)