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 Socket::Address#pretty_print and #inspect #6704

Conversation

straight-shoota
Copy link
Member

No description provided.

@asterite
Copy link
Member

Why? How does it look if you print an array of addresses?

@straight-shoota
Copy link
Member Author

straight-shoota commented Sep 12, 2018

p [Socket::IPAddress.new("127.0.0.1", 80), Socket::IPAddress.new("::1", 8000)]
#before:
# => [Socket::IPAddress(@family=INET, @size=16, @port=80, @address="127.0.0.1", @addr6=nil, @addr4=LibC::InAddr(@s_addr=16777343_u32)), Socket::IPAddress(@family=INET6, @size=28, @port=8000, @address="::1", @addr6=LibC::In6Addr(@__in6_u=LibC::In6AddrIn6U(@__u6_addr8=StaticArray[0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 0_u8, 1_u8], @__u6_addr16=StaticArray[0_u16, 0_u16, 0_u16, 0_u16, 0_u16, 0_u16, 0_u16, 256_u16], @__u6_addr32=StaticArray[0_u32, 0_u32, 0_u32, 16777216_u32])), @addr4=nil)]

# after:
# => [127.0.0.1:80, [::1]:8000]

I don't think there is any reason for #inspect to print all the internal values when the string representation conveys exactly the same information in a human readable format.

@asterite
Copy link
Member

I agree. It should print something like IpAddress(127.0.0.1:80) but definitely not just the address.

@asterite
Copy link
Member

Not sure Ruby has a class for ip addresses, but it would be interesting to see what Ruby does here.

@asterite
Copy link
Member

Here's Ruby:

reqirb(main):001:0> require 'ipaddr'
=> true
irb(main):002:0> ipaddr1 = IPAddr.new "3ffe:505:2::1"
=> #<IPAddr: IPv6:3ffe:0505:0002:0000:0000:0000:0000:0001/ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff>
irb(main):003:0> ipaddr3 = IPAddr.new "192.168.2.0/24"
=> #<IPAddr: IPv4:192.168.2.0/255.255.255.0>
irb(main):004:0> ipaddr3 = IPAddr.new "192.168.2.0"
=> #<IPAddr: IPv4:192.168.2.0/255.255.255.255>

As I mentioned in some other PR, inspect should produce an output that's clearly delimited. But "[1]::2]" looks a bit like a broken array literal.

@straight-shoota
Copy link
Member Author

There is IPAddr but it's quite a bit different from Crystal's IPAddress which is essentially a socket address consisting of IP and port. In Ruby, it's only an IP address but with subnet mask.

p IPAddr.new "3ffe:505:2::1" # => #<IPAddr: IPv6:3ffe:0505:0002:0000:0000:0000:0000:0001/ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff>

@straight-shoota
Copy link
Member Author

You're right. Let's make it Socket::IPAddress(127.0.0.1:80).

@RX14
Copy link
Contributor

RX14 commented Sep 12, 2018

Aren't we throwing away a lot of the information here?

@asterite
Copy link
Member

@straight-shoota Oh, and to_s should behave like what you originally proposed for inspect.

Again, in Ruby:

$ irb
irb(main):001:0> require 'ipaddr'
=> true
irb(main):002:0> puts IPAddr.new "192.168.2.0"
192.168.2.0

@straight-shoota
Copy link
Member Author

@RX14 Not really. All the information is expressed in the string representation. The data structures are just pretty complicated which makes it look like there is a whole lot more information in it.

@asterite #to_s has been like this before and it doesn't change with this PR. It's even used by inspect.
Additionally to the Ruby string representation, there is also a port.

@straight-shoota
Copy link
Member Author

Maybe we should really rename IPAddress to IPSocketAddress or SocketAddress to make it more clear, that it is not just an IP address.

@RX14 RX14 merged commit ee71249 into crystal-lang:master Sep 12, 2018
@RX14 RX14 added this to the 0.27.0 milestone Sep 12, 2018
@straight-shoota straight-shoota deleted the jm/feature/socket_address-pretty_print branch September 12, 2018 20:47
ezrast pushed a commit to ezrast/crystal that referenced this pull request Oct 2, 2018
* Add Socket::IPAddress#pretty_print and #inspect

* fixup! Add Socket::IPAddress#pretty_print and #inspect
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants