Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GetUnderlying function to ErrorWithPos #260

Merged
merged 1 commit into from Aug 2, 2019
Merged

Add GetUnderlying function to ErrorWithPos #260

merged 1 commit into from Aug 2, 2019

Conversation

bufdev
Copy link
Contributor

@bufdev bufdev commented Aug 2, 2019

I need access to the underlying error without any source information, this exposes it. Everything works if you just add the function, I added specific tests cases as well though.

This effectively makes ErrorWithPos equal to the old ErrorWithSourcePos in terms of what it exposes, but as an interface.

@tie
Copy link
Contributor

tie commented Aug 2, 2019

Hi! That’s a nice feature, but you should take a look at the current developments in Go. The next release (Go 1.13) will have errors.Unwrap function that unwraps the underlying error if it the wrapper implements interface { Unwrap() error }.

Copy link
Owner

@jhump jhump left a comment

Choose a reason for hiding this comment

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

Good catch.

@jhump jhump merged commit 0a6fd46 into jhump:master Aug 2, 2019
@bufdev
Copy link
Contributor Author

bufdev commented Aug 2, 2019

That's fine @tie, but protoreflect is meant to be compatible with previous Golang versions, thanks for checking this over though, always good to have new people interested in this amazing library by @jhump.

@bufdev bufdev deleted the get-underlying branch August 2, 2019 17:55
@jhump
Copy link
Owner

jhump commented Aug 2, 2019

Since this PR isn't actually released, it may be nice to rename the method to Unwrap. As @tie pointed out, that will allow it to transparently work with errors.Unwrap for people that are using Go 1.13.

@tie
Copy link
Contributor

tie commented Aug 2, 2019

@pedgeio, simply renaming GetUnderlying to Unwrap would make it forward-compatible too.

@bufdev
Copy link
Contributor Author

bufdev commented Aug 2, 2019

Yea I'm down with that, want me to put up a PR or can you push it quickly?

@jhump
Copy link
Owner

jhump commented Aug 2, 2019

Do you mind? I actually have some other changes I'm working on at the moment and don't want to get side-tracked. If you don't have a chance, I can get to it later today.

@bufdev
Copy link
Contributor Author

bufdev commented Aug 2, 2019

Yep I'll do it right now

@bufdev
Copy link
Contributor Author

bufdev commented Aug 2, 2019

#261

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants