-
-
Notifications
You must be signed in to change notification settings - Fork 744
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
Fix typos in dataloader docs / improve grammatical consistency #7645
base: main
Are you sure you want to change the base?
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.
DataLoader is a fixed term:
https://github.com/graphql/DataLoader
implement suggested comment for capitalization Co-authored-by: Michael Staib <[email protected]>
implement suggested comment for capitalization Co-authored-by: Michael Staib <[email protected]>
implement suggested comment for capitalization Co-authored-by: Michael Staib <[email protected]>
fix capitalization
fix capitalization
fix capitalization
fix capitalization
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.
It will be up to the team to decide if this change is useful or not, but I noted a few places pluralization should be used.
@@ -113,13 +113,13 @@ It executes resolvers until the queue is empty and then triggers the data loader | |||
|
|||
# Data Consistency | |||
|
|||
Dataloader do not only batch calls to the database, they also cache the database response. | |||
DataLoader do not only batch calls to the database, they also cache the database response. |
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.
Should be plural, Dataloaders
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.
Yeah these changes are literally what I originally suggested and I got feedback to NOT make them plural and instead use the PascalCase version. I don't have a horse in the race nor do I know what the correct usage is; I was just trying to help out and make things more consistent because this store out to me when I was reading the docs. I'm happy to go through and make changes with some overall guidance; didn't mean to take a bunch of resources with a casing change.
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.
Gotcha! I was just tossing in an opinion. I'm not the one to give the official guidance.
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.
According to Michael, it's an invariant noun (like middleware), so there's no plural form with an s
suffix. 🤷♂️
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.
Thanks both; so I think Michael would still need to approve since it's blocked with "changes requested"? Or maybe that's not an accurate interpretation? I'm a little rusty on this GitHub workflow.
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.
Michael will merge this when he gets a chance. Thanks.
A data loader guarantees data consistency in a single request. | ||
If you load an entity with a data loader in your request more than once, it is given that these two entities are equivalent. | ||
|
||
Data loaders do not fetch an entity if there is already an entity with the requested key in the cache. | ||
|
||
# Types of Data loaders | ||
# Types of DataLoader |
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.
It could be argued that this is conceptually referring to a "Data Loader" thing, but not THE DataLoader
identifier. Either way, should be plural.
Summary of the changes (Less than 80 chars)
Closes #bugnumber (in this specific format)