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

Reformat the parts of an execution request #1090

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

benjie
Copy link
Member

@benjie benjie commented Apr 5, 2024

This PR adds names for each of the "pieces of information" that make up an execution request, and then shows how these values are passed into the ExecuteRequest() algorithm.

This is extracted from #976 as discussed at last night's working group. @leebyron
mentioned that this change adds clarity and is a positive editorial change; splitting this change out will help keep the "extensions" PR focused as it undergoes a few iterations.

cc @michaelstaib

@benjie benjie added the ✏️ Editorial PR is non-normative or does not influence implementation label Apr 5, 2024
Copy link

netlify bot commented Apr 5, 2024

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 31cec49
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/660fb8004cf2800008645c69
😎 Deploy Preview https://deploy-preview-1090--graphql-spec-draft.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@mjmahone mjmahone left a comment

Choose a reason for hiding this comment

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

Approving as this is pure win as-is, feel free to ignore the nits


Given this information, the result of {ExecuteRequest()} produces the response,
to be formatted according to the Response section below.
- {schema}: The schema to use, typically solely provided by the GraphQL service.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit and not-this-PR: GraphQL service is mildly ambiguous, it's the execution service or the executor or something like that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also nit: GraphQL Service capitalized like below?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We use this term across the spec I believe

may contain {FragmentDefinition}.
- {operationName} (optional): The name of the Operation in the Document to
execute.
- {variableValues} (optional): Values for any Variables defined by the
Copy link
Contributor

Choose a reason for hiding this comment

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

nit and not-this-pr: it's not quite any variable: as you can omit some and must include others, but rather Values for a subset of Variables... but I'm not sure I can describe that in a more clarifying less ambiguous way.

@leebyron leebyron merged commit 7485a34 into graphql:main Jun 6, 2024
4 of 5 checks passed
@benjie benjie deleted the request-details branch June 7, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Editorial PR is non-normative or does not influence implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants