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

Docs: some language cleanup in readme and API reference #2680

Merged
merged 3 commits into from
Jun 30, 2020

Conversation

ralph-mcteggart
Copy link
Contributor

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.

Copy link
Contributor

@danielrearden danielrearden left a 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
Copy link
Contributor

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.

Copy link
Contributor Author

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:
Copy link
Contributor

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.

Copy link
Contributor Author

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

@@ -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
Copy link
Contributor

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...

Copy link
Contributor Author

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:
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: "the visitor API"

@IvanGoncharov IvanGoncharov merged commit 11505d7 into graphql:master Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants