Add missing nil guard in valid_hostname?
#160
Conversation
Regression from twingly#158
| return false if hostname.nil? | ||
|
|
||
| # No need to check the TLD, the public suffix list does that | ||
| labels = hostname.split(DOT)[0...-1].map(&:to_s) |
There was a problem hiding this comment.
Just some alternatives. I think current is just as good as them, but options are always fun!
To avoid work later on, not sure if hostname can ever be "" though:
| return false if hostname.nil? | |
| # No need to check the TLD, the public suffix list does that | |
| labels = hostname.split(DOT)[0...-1].map(&:to_s) | |
| return false if hostname.to_s.empty? | |
| # No need to check the TLD, the public suffix list does that | |
| labels = hostname.split(DOT)[0...-1].map(&:to_s) |
or even
| return false if hostname.nil? | |
| # No need to check the TLD, the public suffix list does that | |
| labels = hostname.split(DOT)[0...-1].map(&:to_s) | |
| # No need to check the TLD, the public suffix list does that | |
| labels = hostname.to_s.split(DOT)[0...-1].map(&:to_s) |
I used to like the, latter, "benign value approach", but not sure I do these days :)
There was a problem hiding this comment.
I thought of doing to_s (and guard against the empty string) but opted not to because the code already handles empty string. Sure, avoiding unnecessary work could be good. I will leave it to someone else to profile if they want. :)
There was a problem hiding this comment.
We use to use guard clauses in situations like these. I'm fine with both the committed solution and the first suggestion here, so let's just go with what's already committed :D
|
A release with this fix would be great :) |
v7.0.1 🎉 |
Regression from #158