Skip to content

Conversation

@PeterIvanov
Copy link
Collaborator

The only hiccup here is that code for earlier versions must remain unchanged, it doesn't look all that pretty in source code.
Also, I could use any ideas for more tests.

@PeterIvanov PeterIvanov requested a review from g7r February 24, 2021 13:56
@g7r
Copy link
Contributor

g7r commented Feb 24, 2021

@PeterIvanov, I suggest that public API functions shouldn't be located under //+build .... The reason for that is to not separate documentation for different Go versions. We should document that the behaviour of these functions depends on Go version.

t.Run("LayeredDecorateAgain", func(t *testing.T) {
err := fmt.Errorf("error test: %w", Decorate(io.EOF, "test"))
require.True(t, errors.Is(err, io.EOF))
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should cover opaque wrap here?

@PeterIvanov
Copy link
Collaborator Author

@g7r as implementation stands now, I could move only burrowForTyped (renamed) under build tag, but it may not be better for core readability. But since implementation changed, it may be better just to keep them separate, and in some later release simply drop support of go version prior to 1.13. At this point, code structure would become plain again.

@g7r
Copy link
Contributor

g7r commented Feb 25, 2021

@PeterIvanov, I suggest something like that:

--- interface.go // no build tag // godoc stating that the behaviour is different when using 1.12 and below and 1.13 and above Go versions. func IsOfType(...) bool { return isOfType(...) } --- impl_1.12.go //+build !go1.13 func isOfType(...) bool { // actual implementation for 1.12 and below } --- impl_1.13.go //+build go1.13 func isOfType(...) bool { // actual implementation for 1.13 and above }

We may inline implementation after dropping 1.12 Go version support. BTW, why delay dropping 1.12 support? Maybe we should use the precedent to drop it? 1.12 is two years old today https://blog.golang.org/go1.12 :)

@PeterIvanov
Copy link
Collaborator Author

It would be quite ironic do drop 1.12 at the exact same moment as errorx finally catches up with go 1.13 :) There's no dire need for that at this point, so I would at least delay a little bit.

@PeterIvanov PeterIvanov requested a review from g7r February 25, 2021 09:24
Copy link
Contributor

@g7r g7r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@PeterIvanov PeterIvanov merged commit 19776b6 into unstable_master Feb 25, 2021
@PeterIvanov PeterIvanov deleted the feature/1.13_errors_unwrap branch February 25, 2021 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants