Add non overlapping ranged annotations #7
Conversation
annot_test.go
Outdated
| if (err != nil) != tt.wantErr { | ||
| t.Errorf("Write() error = %v, wantErr %v", err, tt.wantErr) | ||
| if tt.wantErr != nil { | ||
| if tt.wantErr.Error() != err.Error() { |
There was a problem hiding this comment.
I would have used
| if tt.wantErr.Error() != err.Error() { | |
| if !errors.Is(err, wantErr) { |
There was a problem hiding this comment.
Thanks for your comment. I tried it first with errors.Is() and it didn't work. Now I figured out that I need to implement an Is() method for the custom errors.
There was a problem hiding this comment.
I don't think you have, but also should, do that.
The right way is to add a Unwrap method to your custom error
Look at least the implementation
https://cs.opensource.google/go/go/+/refs/tags/go1.22.4:src/errors/wrap.go;l=44
So you should add an Unwrap and use errors.Is here
There was a problem hiding this comment.
What error should be unwrapped if there is no inner error?
errors.Is() is used in the latest change. Maybe it is not the right way to implement Is() method. But how does it work instead?
annot.go
Outdated
| b.WriteString(strings.Repeat(" ", a.Col-widthWritten)) | ||
| if a.Col == a.pipeColIdx { | ||
| b.WriteString("├") | ||
| } else { |
There was a problem hiding this comment.
This multiple if-else branching is complicated and it's uneasy to see what it does
Could you move some of them on simpler functions maybe?
There was a problem hiding this comment.
I'm also fan of early returns and less else. In this case I removed the parent if else with a continue statement.
For this particular statement I don't see any simplification. It would pack the logic in a box with self written note on it (function name). I try to avoid logic indirection behind custom names.
There was a problem hiding this comment.
OK, everyone has his coding preferences 😅😂🤣
There was a problem hiding this comment.
Thanks for your input. Putting something in a box and label it, does not always help readability. Indirection and an imaged name by the author can complicate things. In this case better an ugly else as a own made up name. An example would help to understand what you mean and what could be improved.
meyermarcel
force-pushed
the
allow-non-overlapping-ranged-annotations
branch
from
8f291e5
to
f8dcda1
Compare
meyermarcel
force-pushed
the
allow-non-overlapping-ranged-annotations
branch
from
f8dcda1
to
2899cad
Compare
No description provided.