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

Export GraphQL error types #35

Merged
merged 4 commits into from May 10, 2022
Merged

Export GraphQL error types #35

merged 4 commits into from May 10, 2022

Conversation

heaths
Copy link
Contributor

@heaths heaths commented May 9, 2022

Similar to HTTPError and HTTPErrorItem already (which was actually HttpErrorItem), export GQLError (was GQLErrorREsponse) and GQLErrorItem (was GQLError). This makes it possible for callers to check the errors and react accordingly, as in cli/cli#5588 where certain GraphQL errors could be filtered out.

@heaths
Copy link
Contributor Author

heaths commented May 9, 2022

A specific use case is a work around for community/community#16168. A ProjectNext object can exist in either an Organization or a User if not already linked to a Repository. But to minimize the number of requests I would ideally query both the root organization and user to figure out a ProjectNext.id. In fact, given both an owner and repo name, I could query all three root objects and use whichever ProjectNext I can, but I need to ignore the other 2 errors.

Technically, the current implementation still populates the response interface{} despite returning an error, but this may not always be the case since its atypical. Relying solely on the error message is also fragile and again relies on implementation details. Instead, just like RESTClent, export the GQLError so they can be used by the caller.

Copy link
Contributor

@samcoe samcoe left a comment

Choose a reason for hiding this comment

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

This looks good to me. Only comment would be that I think we should rename HttpErrorItem -> HTTPErrorItem in this PR as well since we are already here and it was a typo to begin with. Since go-gh is beta I don't think there is much controversy with fixing that small mistake.

pkg/api/client.go Outdated Show resolved Hide resolved
gh_test.go Outdated Show resolved Hide resolved
@heaths
Copy link
Contributor Author

heaths commented May 9, 2022

I did rename it in another commit but realized I forgot to push it. 😣

@samcoe samcoe self-assigned this May 9, 2022
Copy link
Contributor

@samcoe samcoe left a comment

Choose a reason for hiding this comment

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

LGTM!

@samcoe samcoe enabled auto-merge (squash) May 10, 2022 06:11
@samcoe samcoe merged commit 8d34617 into cli:trunk May 10, 2022
@heaths heaths deleted the export-gqlerror branch September 13, 2022 05:29
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

2 participants