-
Notifications
You must be signed in to change notification settings - Fork 2k
Docs: some language cleanup in readme and API reference #2680
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmcteggart-r7 Thanks so much for submitting this PR! I left a couple of comments to address; otherwise, it looks great 😄
README.md
Outdated
@@ -13,7 +13,7 @@ Looking for help? Find resources [from the community](https://graphql.org/commun | |||
|
|||
## Getting Started | |||
|
|||
An overview of GraphQL in general is available in the | |||
An overview of GraphQL, in general, is available in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: This changes the meaning of the original sentence.
"an overview of GraphQL in general" is equivalent to "a general overview of GraphQL". By adding the commas, we are changing the meaning to "an overview of GraphQL, in most situations".
The original sentence is syntactically valid, although changing it to "A general overview of GraphQL" would be more succinct and clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Updated!
README.md
Outdated
@@ -142,7 +141,7 @@ GraphQL.js is [MIT-licensed](./LICENSE). | |||
|
|||
### Credits | |||
|
|||
The `*.d.ts` files in this project are based on [DefinitelyTyped](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/54712a7e28090c5b1253b746d1878003c954f3ff/types/graphql) definitions written by: | |||
The `*.d.ts` files in this project are based on [DefinitelyTyped](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/54712a7e28090c5b1253b746d1878003c954f3ff/types/graphql). Definitions written by: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: This changes the original meaning of the sentence.
"DefinitelyTyped" describes what definitions the files are based on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops read this differently first time. Fixed
docs/APIReference-Language.md
Outdated
@@ -106,9 +106,9 @@ export class Source { | |||
``` | |||
|
|||
A representation of source input to GraphQL. The name is optional, | |||
but is mostly useful for clients who store GraphQL documents in | |||
but is most useful for clients who store GraphQL documents in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Either word would be correct, but they convey different meanings ("generally" vs "to the greatest degree").
I think dropping the word altogether would make the sentence clearer and make the use of "but" justifiable. We should also either drop the comma before "but" or add "it" after "but". A comma is only used if joining two independent clauses. I think this would be fine:
The name is optional, but it is useful...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed on omitting 👍 . Also updated
@@ -233,7 +233,7 @@ instead provide functions named the same as the kinds of AST nodes, or | |||
enter/leave visitors at a named key, leading to four permutations of | |||
visitor API: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: "the visitor API"
Noticed a small grammar mistake. I went to fix it and then noticed some more so I'm including them also. Cleaner language hopefully makes for easier understanding.