Improved error message fallback logic #35
Conversation
tadam313
changed the title
tadam313
changed the title
|
also lint failed during dependency fetch however I don't have permission to re-run it, can you please do that @joshdk? |
There was a problem hiding this comment.
Hi @tadam313 👋
Thank you for your contribution! (and for finding edge-cases in the wild) Overall looks fine, left 2 minor comments.
Additionally, I'm going to look into the lint failure; I retriggered the lint stage, and it failed in the same way (but I don't think it's related to anything you committed). It's probably due to how we're go get-ing golangci-lint.
Cheers!
|
@joshdk I believed, addressed every comments, can you please take another look? |
types.go
Outdated
| if strings.TrimSpace(err.Type) != "" { | ||
| return err.Type | ||
| } | ||
|
|
||
| return err.Body |
There was a problem hiding this comment.
At the very end, do you want to return err.Type with no check? The value of err.Body is guaranteed to be blank (or at best all whitespace characters) by this point.
There was a problem hiding this comment.
Well, my thinking was "try all the fields, if everything is still empty then return Body". In this case we can maintain backward compatibility, since so far only Body was used. I'm not sure if we need to change that behaviour. WDYT @joshdk ?
There was a problem hiding this comment.
Since err.Body is both the first thing (preferably) returned, and the fallback, it will be blank in the latter case. Concretely, I was thinking of something like:
switch {
case strings.TrimSpace(err.Body) != "":
return err.Body
case strings.TrimSpace(err.Message) != "":
return err.Message
default:
return err.Type
}There isn't really a way to maintain data back-compat in this case (as the function can possibly return more than just that single value now), but I would be fine with the above refactor 🙂
There was a problem hiding this comment.
all right, sounds good to me 👍
|
Any thoughts, or things I can help with @tadam313? Cheers! |
|
@joshdk all good, sorry just been busy in last few days, but I did the modifications you requested, also rebased on master thanks fixing the dependency issue 👍 |
joshdk
changed the title
Hi @joshdk! We came across an issue when junit XML failure report contains
messagebut no meaningfulbodyin it. In that scenario we'd ideally use themessageas a fallback ortypeto display the error rather than the "empty" body. I've created a PR to address it, also open for any suggestion/requests, happy to discuss this further :)