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 error message for missing required argument in field or directive #1554

Merged

Conversation

everdimension
Copy link
Contributor

No description provided.

@@ -27,7 +27,7 @@ export function missingFieldArgMessage(
): string {
return (
`Field "${fieldName}" argument "${argName}" of type ` +
`"${type}" is required but not provided.`
`"${type}" is required, but not provided.`
Copy link
Member

Choose a reason for hiding this comment

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

@everdimension Thanks for PR 👍
I'm not a native speaker myself but not provided looks like a dependent sentence for me so it looks like this sentence doesn't need comma based on this article:
https://www.grammarly.com/blog/comma-before-but/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither am I, but this message seems a bit off for some reason.

After further investigation, it seems that the comma before "but" is not obligatory in the english language when the subject is not repeated in the second clause.

But now that I've had a second look, perhaps the problem is the omitted verb.
So the correct message should sound something like this:

"${type}" is required but was not provided.
or
"${type}" is required, but it was not provided. (with comma, because "it" is repeated in the second clause)

But again, perhaps omitting the auxiliary verb is permissible for system messages like this one.


Anyway, this is definitely a nitpick. I can update my PR and add was instead of the comma if you agree that this is an improvement. Otherwise feel free to close this

Copy link
Member

Choose a reason for hiding this comment

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

@everdimension I like was variant 👍

Also would be great to also get it reviewed by a native speaker.

Copy link
Contributor

Choose a reason for hiding this comment

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

The comma should not matter. I think it's a British-ism vs American-ism, and so I don't think it makes a big difference. Despite being an American-English-speaker, commas or not read mostly the same in this case.

I agree that the was variant is nicest.

@IvanGoncharov IvanGoncharov changed the title add missing commas to the validator messages Improve error message for missing required argument in field or directive Feb 5, 2019
@IvanGoncharov IvanGoncharov force-pushed the chore/provided-required-messages branch from baaafd3 to 1765de8 Compare February 5, 2019 16:02
@IvanGoncharov IvanGoncharov added the PR: bug fix 🐞 requires increase of "patch" version number label Feb 5, 2019
@IvanGoncharov IvanGoncharov added this to the v14.2.0 milestone Feb 5, 2019
@IvanGoncharov IvanGoncharov merged commit 82851fc into graphql:master Feb 5, 2019
@IvanGoncharov
Copy link
Member

@everdimension I changed it to is required, but it was not provided. and merged 🎉
Thanks for PR and patience 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants