SystemError: Fix inconsistent signature. #11002
Conversation
In `.from_os_error` the code looks like
```
message = self.build_message(message, **opts)
message =
if message
..
else
..
end
```
which clearly doesn't make sense if `build_message` always return a
string.
spec/std/system_error_spec.cr
Outdated
| it "Can build an error from an errno" do | ||
| errno = Errno.new(11) | ||
| error = ::IO::Error.from_os_error(message: nil, os_error: errno) | ||
| error.message.should eq("Resource temporarily unavailable") |
There was a problem hiding this comment.
LibC-specific. It will be interesting to see if it passes CI.
There was a problem hiding this comment.
It doesn't. On Mac it gives "Resource deadlock avoided". I'll change to match against /Resource/ unless someone has a better idea (like a suggestion for a more consistent errno to compare - I don't have access to a mac and I didn't find a list of errno messages it can give).
There was a problem hiding this comment.
BTW, it is a bit strange to get email about failed specs, but that it doesn't show up as broken in the github UI. Is that a known issue?
There was a problem hiding this comment.
Maybe the CI notification is for an older commit?
There was a problem hiding this comment.
Can't be, I got two failure notifications and no obvious sign in the UI. But perhaps it was there but just not obvious enough 🤷
There was a problem hiding this comment.
For the error messages, you can just compare the respective manpages (https://man7.org/linux/man-pages/man3/errno.3.html, https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/intro.2.html, https://www.freebsd.org/cgi/man.cgi?query=errno&sektion=2&format=html, etc.).
I suppose ENOENT should be pretty portable: No such file or directory.
src/system_error.cr
Outdated
| @@ -85,7 +85,7 @@ module SystemError | |||
| # | |||
| # By default it returns the original message unchanged. But that could be | |||
| # customized based on the keyword arguments passed to `from_errno` or `from_winerror`. | |||
| protected def build_message(message, **opts) : String | |||
| protected def build_message(message, **opts) : String? | |||
There was a problem hiding this comment.
Could also add String? restriction to message like in from_os_error.
There was a problem hiding this comment.
Right. That won't affect functionality but I guess it ends up clearer.
straight-shoota
added
kind:bug
yxhuvud
force-pushed
the
fix/system_error_inconsistent_type_signatures
branch
from
7d4c515 to
559844d
Compare
In
.from_os_errorthe code looks likewhich clearly doesn't make sense if
build_messagealways return astring.