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 the content type error message by showing available content types #3532

Merged
merged 2 commits into from
Jun 7, 2019

Conversation

gitlw
Copy link

@gitlw gitlw commented Jun 6, 2019

This change is Reviewable

@gitlw gitlw requested review from manishrjain and a team as code owners June 6, 2019 22:24
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gitlw and @manishrjain)


dgraph/cmd/alpha/http.go, line 204 at r1 (raw file):

	default:
		x.SetStatus(w, x.ErrorInvalidRequest, "Unsupported content type. "+
			"Currently supported content types are application/json, application/graphqlpm")

minor: I think it's safe to remove the currently to make the message a bit shorter. It's implied that the message is referring to the current types.

Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gitlw and @manishrjain)


dgraph/cmd/alpha/http.go, line 203 at r1 (raw file):

	default:
		x.SetStatus(w, x.ErrorInvalidRequest, "Unsupported content type. "+

Let's keep the original spelling of "Content-Type" so it's clearer this is referring to the header name.


dgraph/cmd/alpha/http.go, line 331 at r1 (raw file):

	default:
		x.SetStatus(w, x.ErrorInvalidRequest, "Unsupported content type. "+

Let's keep the original spelling of "Content-Type" so it's clearer this is referring to the header name.

Copy link
Author

@gitlw gitlw left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @danielmai, @manishrjain, and @martinmr)


dgraph/cmd/alpha/http.go, line 203 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Let's keep the original spelling of "Content-Type" so it's clearer this is referring to the header name.

Done.


dgraph/cmd/alpha/http.go, line 331 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Let's keep the original spelling of "Content-Type" so it's clearer this is referring to the header name.

Done.

@gitlw gitlw merged commit 9cac881 into master Jun 7, 2019
@gitlw gitlw deleted the gitlw/improve_content_type_err branch June 7, 2019 00:31
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants