-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix: IPAddress#address returns a nilable #3872
Fix: IPAddress#address returns a nilable #3872
Conversation
@@ -132,10 +132,6 @@ class Socket | |||
end | |||
end | |||
|
|||
private def address(addr) : Nil | |||
# shouldn't happen | |||
end |
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.
How about keeping this method, and have it raise a @addr is not set
exception, and remove the two not_nil!
assertions above? I'm guessing it would be more descriptive should we land in that case.
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.
I'm not sure. Either @addr4
or @addr6
must have been set in #initialize
(otherwise it raises). The compiler can't know about it because that depends on the third-party @family
variable. Let's avoid an overload for an impossible case?
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.
Sounds reasonable 👍
@@ -107,8 +107,8 @@ class Socket | |||
def address |
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.
@ysbaddaden @spalladino Please next time prefer adding a return type annotation here, which makes docs more clear and removes the need to write a typeof
test for this, which is redundant
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.
Ok, that's noted!
The socket refactor led
IPAddress#address
to now return a nilable when it should only ever return aString
.refs ysbaddaden/prax.cr#42