fixing some linter warning and documentation #6
Conversation
golint asks to remove else blocks when returning to improve readability.
|
|
||
| if areAllOfTheSameType(errs...) { | ||
| return WrapMany(transparentWrapper, message, errs...) | ||
| } else { |
There was a problem hiding this comment.
There is a thing here that annoys me a little. Here's a guideline we are trying to comply with here:
https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow
And it goes well when there is a 'negative' branch (greater indentation) and a 'positive' branch (minimal indentation).
Here, on the other hand, there a 2 positive branches, so it seems that changing the code in this manner actually makes it send a wrong message to a reader.
Thoughts?
As for other similar fixes in this PR, I suggest we apply the same rule. If we apply this guideline, at least it should show what is considered a 'positive' codepath.
There was a problem hiding this comment.
You bring up a good point. In the example you highlighted I would argue that !areAllOfTheSameType would be the negative, so the logic should be reversed. As for the others I think the ones in builder.go could also be flipped. Final choice is yours. If you prefer to leave these I can remove them from the PR and just leave the doc fix. Thoughts?
There was a problem hiding this comment.
The main point is whether or not linter is being a little too zealous here. I originally felt like it is, and that's why those reports remained unsolved until now. But if you feel otherwise, I'm ready to reconsider.
Could you please modify this PR so that this rule of positive path is best respected, and then I'll make comments if I disagree with some of those choices?
PeterIvanov
left a comment
PeterIvanov
left a comment
There was a problem hiding this comment.
Looks good to me.
Please address one small remaining issue and I'm ready to merge this PR.
switch.go
Outdated
| package errorx | ||
|
|
||
| // CaseNoTrait is a synthetic type used in TypeSwitch, signifying a presence of non-nil error of some other type. | ||
| // NotRecognisedType is a synthetic type used in TypeSwitch, signifying casting error to *Error failed. |
There was a problem hiding this comment.
Actually, not only that. Any type not expected in a switch, too.
So I suggest leaving the second half unchanged.
|
If you would like me to squash those back down to two commits lmk. Btw, cool project. |
PeterIvanov
left a comment
PeterIvanov
left a comment
There was a problem hiding this comment.
Thanks!
I don't see any need for a squash, so I'll merge this as it is if it's OK with you.
2 participants
No description provided.