-
Notifications
You must be signed in to change notification settings - Fork 98
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
Improve reverse IP handling #60
base: master
Are you sure you want to change the base?
Conversation
Thanks @gauthier-delacroix, I will be looking at this soon to assess the impact/performance but overall at a quick glance, it looks pretty solid and complete. |
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.
Thank you for this PR. I have just joined the project 😄
There are a few minor issues, may I ask you to fix them? Also rebasing on top of master would fix conflicts and allow Travis-CI to run the test suite.
You also can allow the project's contributors to add commits to your branch if you do not have time to do this. In this case, please ping me 😉
Thanks!
@@ -49,6 +55,10 @@ def IPAddress::parse(str) | |||
end | |||
|
|||
case str | |||
when /\.in-addr.arpa$/ |
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.
Missing \
: /\.in-addr\.arpa$/
@@ -49,6 +55,10 @@ def IPAddress::parse(str) | |||
end | |||
|
|||
case str | |||
when /\.in-addr.arpa$/ | |||
IPAddress::IPv4.from_reverse(str) | |||
when /\.ip6.arpa$/ |
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.
Missing \
: /\.ip6\.arpa$/
# #=> "172.16.15.0/24" | ||
# | ||
def self.from_reverse(reverse) | ||
address = reverse.gsub(/^(.*)\.in-addr.arpa$/, '\1').split('.').reverse |
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.
Same here for consistency
# #=> "3ffe:505::/40" | ||
# | ||
def self.from_reverse(reverse) | ||
address = reverse.gsub(/^(.*)\.ip6.arpa$/, '\1').split('.').reverse |
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.
One more time here
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.
This was a "Request changes" review, not a "Comment" one…
Hi, |
On Wed, Aug 30, 2017 at 04:30:48PM +0000, Gauthier Delacroix wrote:
Not sure I'll have time to update a 2 years old PR soon...
That's why I proposed you to allow maintainers to add commits into your
branch: on the PR page, at the bottom of the right column, under the
participants list, check "Allow edits from maintainers" 😉.
|
Allows IPv6 reversing, network reversing (through
#reverse_network
, for arpa classes handling) and v4/v6 arpa strings toIPAddress
conversion.Added
IPAddress#parse
the ability to handle arpa strings and return corresponding network addresses.I thought about handling arpa strings in
#new
but I prefered keeping this feature into#from_reverse
methods.Added
#valid_ipv4_reverse?
and#valid_ipv6_reverse?
to check those strings.I think reverse handling is quite complete now...