Use Exception#message getter instead of @ivar to allow overloading #3194
Conversation
|
@Sija would you mind adding a spec that test customization of Thank you. |
|
@luislavena Sure thing! I don't understand tho why there's nil assertion failing, could you have a look, please? |
|
I found what's going on. In b3943d5#diff-8a92cd1599650bc7c991a21c66fbc77bR171 there should be There was not specs for I will do one more comment inline and after squashing the commits in a single commit we should be fine :-). |
| require "spec" | ||
|
|
||
| class FooError < Exception | ||
| def message |
There was a problem hiding this comment.
Due to how you use the specs I will suggest
class FooError < Exception
def message
"#{super || ""} -- bar!"
end
endSo that the spec tests that the base message implementation returns the value used at creation and that to_s / inspect_with_backtrace use the method rather than the ivar.
find a full sample at https://play.crystal-lang.org/#/r/17lx
|
@Sija if you may, include a fix for #3194 (comment) so the specs pass. Then if you don't know how to squash the PR in a single commit check here. Thanks! |
|
@bcardiff Since specs are still failing should I include the fix for |
|
@Sija yes please. I tried to be clear on that, sorry (& thanks!). |
|
@bcardiff Oki doki, I wasn't sure I grok your intention ;) Should be GTG now. |
| class Exception | ||
| def inspect_with_backtrace(io : IO) | ||
| io << message << " (" << self.class << ")\n" | ||
| backtrace?.try &.each do |frame| |
There was a problem hiding this comment.
Wouldn't be better this fix is part of the code itself (src/exception.cr) and not just the specs? /cc @bcardiff
There was a problem hiding this comment.
Definitely. @Sija please add the ? exception.cr and remove the Exception class in exception_spec.cr, otherwise the code that is been tested is not the actual code. I made the shared snippet to illustrate and quick check if the error was the missing ?.
|
@bcardiff done. |
|
Thanks! |
Suggestion as in the title. It would match Ruby behavior too.