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

Improve CloseError API #128

Closed
nhooyr opened this issue Aug 31, 2019 · 13 comments · Fixed by #133
Labels

Comments

@nhooyr
Copy link
Owner

@nhooyr nhooyr commented Aug 31, 2019

I think the current API is a little obnoxious since it forces you to use xerrors.As to get the close error code which is very annoying.

Also people forget to use xerrors.As, see https://github.com/sumorf/deribit-api/blob/6b581052e02d0808bed0bdc7a04f3dc4582e70d3/stream.go#L30

@sumorf

I don't like the ceremony involved with xerrors.As so I'd like to provide some sort of convenience API to get the error code.

@nhooyr nhooyr added proposal discussion and removed proposal labels Aug 31, 2019
@nhooyr

This comment has been minimized.

Copy link
Owner Author

@nhooyr nhooyr commented Sep 1, 2019

Any thoughts @coadler @ammario?

@nhooyr

This comment has been minimized.

@coadler

This comment has been minimized.

Copy link
Contributor

@coadler coadler commented Sep 1, 2019

I don't mind using xerrors.As to get extra information. Since it's moving to std errors in 1.13 I think that approach might start being more common, though I wouldn't see an issue with adding a helper function

@andersfylling

This comment has been minimized.

Copy link

@andersfylling andersfylling commented Sep 1, 2019

I would prefer a helper method. At least until 1.13 is released and a std approach exists.

Edit: right now I've just imported xerrors to handle it properly. You could just show how to properly handle your error types as now many are used to xerrors yet.

@nhooyr

This comment has been minimized.

Copy link
Owner Author

@nhooyr nhooyr commented Sep 1, 2019

Yea I suppose the simplest approach for now is to just add an example of accessing CloseError to the README.

What do you guys think of unexporting the CloseError struct all together and exporting:

func CloseError(err error) (code StatusCode, reason string, ok bool)

Would be pretty much impossible to misuse and consumers would never have to think about xerrors.As and how to grab the error properly.

@nhooyr

This comment has been minimized.

Copy link
Owner Author

@nhooyr nhooyr commented Sep 1, 2019

imo its better if xerrors.As is an implementation detail and not something consumers think about.

@andersfylling

This comment has been minimized.

Copy link

@andersfylling andersfylling commented Sep 1, 2019

I would say:
If you want to keep go 1.12 support, then yes.
If you want to use go1.13 then just stick with the .As method, as that will have std support when 1.13 is released.

A compromise would be to keep the error struct exported and provide that helper method (marked deprecated). Then people can choose.

@nhooyr

This comment has been minimized.

Copy link
Owner Author

@nhooyr nhooyr commented Sep 1, 2019

@jba what would you recommend as the public API library author's go for? Is it better to expose the error structure and require consumers use xerrors.As or provide a helper?

@ammario

This comment has been minimized.

Copy link

@ammario ammario commented Sep 2, 2019

It seems like Go intends on everyone using xerrors.As, right? I think a helper method for each error type is excessive duplication.

nhooyr added a commit that referenced this issue Sep 2, 2019
Closes #128
@nhooyr

This comment has been minimized.

Copy link
Owner Author

@nhooyr nhooyr commented Sep 2, 2019

Yes, I think you're right. I've added an example which should help with the confusion.

nhooyr added a commit that referenced this issue Sep 2, 2019
Closes #128
@nhooyr nhooyr closed this in #133 Sep 2, 2019
@jba

This comment has been minimized.

Copy link

@jba jba commented Sep 3, 2019

Sorry for the late reply. I agree with @ammario: no need to duplicate errors.As functionality.

@andersfylling

This comment has been minimized.

Copy link

@andersfylling andersfylling commented Sep 3, 2019

go1.13 is out now, so you can enforce go1.13 as minimum and remove the xerrors pkg.

@nhooyr

This comment has been minimized.

Copy link
Owner Author

@nhooyr nhooyr commented Sep 3, 2019

Will do @andersfylling, opened #134

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.