fix: raise ExecutionError with partial error response
#87
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
Ugh travis-ci is MIA. Care to PR a quick replacement for it with GHA so we can have a working CI? |
Contributor
Author
Yes. Please check out #89 |
QQism
force-pushed
the
qq/fix/partial-success-response
branch
from
afb80e7 to
048159e
Compare
QQism
force-pushed
the
qq/fix/partial-success-response
branch
2 times, most recently
from
e95b71d to
11a026d
Compare
Collaborator
|
Update the CHANGELOG please and we're good to go! Thanks. |
dblock
reviewed
Jun 11, 2022
When the GraphQL response has nested errors, `graphlient` does not raise the `ExcutionError` which is not expected. This commit will check all errors (including nested one) by using `errors.all` and make sure the exception is raised. Some considerations: - Should we use the `Graphlient::Errors::ExecutionError` for partial success response or create a new type of exception to maintain backward compatibility? I tend to think having one type of exception could make things simpler for usage. - Should `graphlient` ignore nested errors and not raise exception? Apparently, the whole point of this commit is about it. I'm working on a sensitive GraphQL client app, and any type of errors should be well aware to proceed to the appropriate next step. References: - https://github.com/github/graphql-client/pull/132#issuecomment-386111906
QQism
force-pushed
the
qq/fix/partial-success-response
branch
from
11a026d to
808a149
Compare
Collaborator
|
I released 0.6.0. |
Collaborator
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the GraphQL response has nested errors,
graphlientdoes not raisethe
ExcutionErrorwhich is not expected. This commit will check allerrors (including nested one) by using
errors.alland make sure theexception is raised.
Some considerations:
Graphlient::Errors::ExecutionErrorfor partialsuccess response or create a new type of exception to maintain
backward compatibility? I tend to think having one type of exception
could make things simpler for usage.
graphlientignore nested errors and not raise exception?Apparently, the whole point of this commit is about it. I'm working on
a sensitive GraphQL client app, and any type of errors should be well
aware to proceed to the appropriate next step.
References: