Modernize error handling #21
Conversation
There was a problem hiding this comment.
LGTM and I approve of these changes!
In fact, I made similar suggestions a while ago:
- Implement compute-optimised Merkle tree #5 (comment)
- Implement compute-optimised Merkle tree #5 (comment) (Implement compute-optimised Merkle tree #5 (comment))
Back then @musalbas preferred to keep the error handling like before. Hence, I'd rather have him approve/decline the changes.
.travis.yml
Outdated
| @@ -1,4 +1,6 @@ | |||
| language: go | |||
| go: | |||
| - 1.14.x | |||
There was a problem hiding this comment.
Is there a reason 1.14.x is used here instead of 1.15.x?
There was a problem hiding this comment.
I don't think there is a particular reason. My suggestion would be to update it to 1.16 unless we want to also test backwards compatibility to old go versions. Then we could add them too. Either way, 1.16 should be in there.
There was a problem hiding this comment.
No reason. Actually I'm going to merge #23, to it doesn't matter anyways.
There was a problem hiding this comment.
Now I remember - the reason was, go.mod is using 1.14.
There was a problem hiding this comment.
Ah that makes sense. Unrelated to the current PR, what are the ramifications of updating go.mod to 1.15 or 1.16? cc @liamsi too
There was a problem hiding this comment.
As far as I remember this would simply indicate that we are using language features that were introduced with 1.16 (or whatever will be used in go.mod) and that we do not plan to support version smaller than the one listed in go.mod.
Btw, these versions do not necessarily need to match (the one in go.mod and the one used for travis). The one in go.mod could be lower. In the IPFS Plugin case, I wanted all versions to match because the plug-in used some x/system package which caused problems. I don't think that's a problem here.
* more idiomatic approach to errors (var instead of struct) * use errors.Is, as it's preferred way to check errors in go 1.13+ See: https://blog.golang.org/go1.13-errors
tzdybal
force-pushed
the
tzdybal/modern_errors
branch
from
213066e to
da27ff7
Compare
| @@ -26,7 +26,7 @@ jobs: | |||
| go.mod | |||
| go.sum | |||
| - name: install | |||
| run: GOOS=linux GOARCH=${{ matrix.goarch }} make build | |||
| run: GOOS=linux GOARCH=${{ matrix.goarch }} go build | |||
There was a problem hiding this comment.
Somehow we missed this during all reviews and testing 😂
|
@musalbas please make a final decision (see: #21 (review)) |
|
Isn't this the wrong naming convention for sentinel value based errors? From the docs you linked, sentinel values start with |
Reference: https://blog.golang.org/go1.13-errors