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

xerrors: adds IsOneOf #1

Closed
wants to merge 1 commit into from
Closed

Conversation

lsiv568
Copy link

@lsiv568 lsiv568 commented Feb 8, 2019

After reading Proposal: Go 2 Error Inspection I absolutely loved the introduction of the Is API. I looked around some of my go codebases and immediately realized how it could improve readability by removing verbose equality checking. After this exercise, I saw how the introduction of an IsOneOf API would further remove verbosity by allowing for the following change:

Before

...
err := test.DoSomething()
if err == test.ErrExpected1 || err == test.ErrExpected2 || err == test.ErrExpected3 {
    return
}
...

After

var expectedErrors = []errors{test.ErrExpected1, test.ErrExpected2,  test.ErrExpected3}
...
err := test.DoSomething()
if xerrors.IsOneOf(expectedErrors, err) {
    return
}
...

I believe the After is both more terse and readable. To that end, I have created this pull request.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@lsiv568
Copy link
Author

lsiv568 commented Feb 8, 2019

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@jba
Copy link
Contributor

jba commented Feb 8, 2019

Thanks so much for your contribution. Unfortunately, we can't accept it in this form.

First, Go development is done with Gerrit (see https://go-review.googlesource.com). We don't accept GitHub PRs.

Second, the xerrors repo is part of a pending proposal. You should first get your suggestion added to the proposal before sending a PR.

Although you're welcome to make your case on the issue, we think it's unlikely that IsOneOf would be accepted. We try to keep our APIs small, and IsOneOf provides too little benefit on top if Is.

Of course, you are free to use it in your own code.

@jba jba closed this Feb 8, 2019
@dmitshur
Copy link
Contributor

dmitshur commented Feb 8, 2019

First, Go development is done with Gerrit (see https://go-review.googlesource.com). We don't accept GitHub PRs.

To clarify, we do accept GitHub PRs (they get imported into Gerrit via GerritBot) in other repositories that are not a part of a pending proposal.

@lsiv568
Copy link
Author

lsiv568 commented Feb 9, 2019

@jba Thanks for the reply! I was under the impression that there was a bridge from GitHub to Gerrit which is why I submitted this PR. This was confirmed by dmitshur above. However, I was not aware that the xerrors package was still under proposal. My apologies for the premature PR!!

While I understand the intent is to keep go core APIs small, I also believe IsOneOf functionality is an extremely valid use case that would be widely used/adopted. In an effort to limit the surface area of the API, one possibility could be to change the signature of the Is API to Is(target error, errTypes ...error). An example of prior art can be see in gorilla/websocket. While this does introduce a different order of precedence with other function signatures, it meets both the objective of a small API surface area and broadens the use cases for the API. Thoughts?

@jba
Copy link
Contributor

jba commented Feb 9, 2019

Thanks for your suggestion. Please make it on the issue.

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