Skip to content

8 - Provide clearer error for non-200 status codes without JSON - #19

Merged
matryer merged 3 commits into
machinebox:masterfrom
erutherford:8-check-status-code
Oct 16, 2018
Merged

8 - Provide clearer error for non-200 status codes without JSON#19
matryer merged 3 commits into
machinebox:masterfrom
erutherford:8-check-status-code

Conversation

@erutherford

@erutherforderutherford commented Aug 31, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

What

  • This adds a check for a non-200 status code if there's a failure to unmarshal the response from the server.
  • Added tests to confirm the current behavior of returning the first error in the index if it is able to parse the response.

#8

Eric Rutherford
adding a check if there is a failure to parse and returning an error …
9835de6
…about the status code if it was not 200
accounting for both paths where there might be a non-200 response
@erutherford

erutherford commented Sep 11, 2018

Copy link
Copy Markdown
ContributorAuthor

@dahernan is it possible you'd be able to take a look at this? I'm definitely open to suggestions on the PR, I was trying to maintain the current behavior, but account for errors where there isn't a JSON response with as little modification as possible.

@dahernan

dahernan commented Sep 11, 2018

Copy link
Copy Markdown
Contributor

Sorry I have no context for this, maybe @matryer could help

@matryer

matryer commented Sep 11, 2018 via email

Copy link
Copy Markdown
Contributor

@erutherford

erutherford commented Sep 11, 2018

Copy link
Copy Markdown
ContributorAuthor

I went based of the last couple merges, sorry for the incorrect ping. Thanks for checking out the PR and providing this client.

matryer
matryer suggested changes Oct 15, 2018
View reviewed changes
Comment threadgraphql.go Outdated
"github.com/pkg/errors"
)

const errorNon200Template = "graphql: server returned a non-200 status code: %v"

@matryermatryerOct 15, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This feels like an unnecessary constant to me. Let's just duplicate the string for now?

@matryermatryerOct 15, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(Great PR btw)

@erutherforderutherfordOct 15, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

thanks @matryer, I pulled the constant and duplicated the string.

@erutherford

erutherford commented Oct 15, 2018

Copy link
Copy Markdown
ContributorAuthor

Looks like it's failing because of this, updating my branch.
golang/lint#415

@matryer
matryer merged commit 06f92ef into machinebox:masterOct 16, 2018
@matryer

matryer commented Oct 16, 2018

Copy link
Copy Markdown
Contributor

Thanks @erutherford, nice work.

@erutherford
erutherford deleted the 8-check-status-code branch October 16, 2018 15:19
This was referenced Sep 23, 2019
Open
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@matryermatryermatryer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@erutherford@dahernan@matryer