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.

Sign up for GitHub

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

Jump to bottom

Improve description for @oneOf directive #3937

Merged
merged 6 commits into from
Sep 1, 2023
Merged

Improve description for @oneOf directive #3937

merged 6 commits into from
Sep 1, 2023

Conversation

spawnia
Copy link
Member

@spawnia spawnia commented Jul 17, 2023

Follow up from #3513.

I think it is fine for the spec text in graphql/graphql-spec#825 to refer to OneOf Input Object, as the spec explains what that is in another section. In a directive definition, that explanation by itself does not make much sense without prior knowledge.

Channeling the RFC champion @benjie.

@spawnia
Improve description for @oneOf directive
0777407 
Follow up from graphql#3513.

I think it is fine for the spec text in graphql/graphql-spec#825
to refer to `OneOf Input Object`, as the spec explains
what that is in another section. In a directive definition, that explanation by itself
does not make much sense without prior knowledge.
@netlify
Copy link

netlify bot commented Jul 17, 2023 •
edited
Loading

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit f1a3b34
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/64b8e87b87525f0008a8d23b
😎 Deploy Preview https://deploy-preview-3937--compassionate-pike-271cb3.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.

@github-actions
Copy link

Hi @spawnia, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@spawnia spawnia mentioned this pull request Jul 17, 2023
Open
yaacovCR
yaacovCR reviewed Jul 17, 2023
View reviewed changes
Copy link
Contributor

@yaacovCR yaacovCR left a comment

Choose a reason for hiding this comment

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

does it work better with the "the" article?

src/type/directives.ts Outdated Show resolved Hide resolved
src/utilities/__tests__/printSchema-test.ts Outdated Show resolved Hide resolved
@benjie
Copy link
Member

benjie commented Jul 18, 2023

Indicates that exactly one field must be supplied and not `null`.

The current wording (after the latest edits) is acceptable, but it has the bracketing problem: (exactly one field must be supplied) and (not null) versus exactly one field must be (supplied and not null). The latter interpretation is incorrect: {a: null, b: 2} supplies two fields, but has exactly one field that is both "supplied and not null".

The original wording was clearer in my opinion. That said, happy to see it merged either way.

@IvanGoncharov
Copy link
Member

@spawnia Can you please address @benjie last comment.
Otherwise ready to be merged 👍

@benjie

This comment has been minimized.

Sign in to view
@github-actions
Copy link

@github-actions publish-pr-on-npm

@benjie The latest changes of this PR are available on NPM as
graphql@17.0.0-alpha.2.canary.pr.3937.8e773a04d8041ffc00a1550e8c6688e01ba11832
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-3937

@benjie benjie mentioned this pull request Aug 29, 2023
Open
@benjie
Copy link
Member

benjie commented Aug 29, 2023

Example of using this can be found here: graphql/graphql-spec#825 (comment)

@IvanGoncharov IvanGoncharov merged commit acf05e3 into graphql:main Sep 1, 2023
sakesun pushed a commit to sakesun/graphql-js that referenced this pull request Sep 1, 2023
@spawnia @sakesun
Improve description for @oneOf directive (graphql#3937)
4846148
@IvanGoncharov IvanGoncharov added the PR: bug fix 🐞 requires increase of "patch" version number label Sep 6, 2023
benjie pushed a commit that referenced this pull request Jun 21, 2024
@spawnia @benjie
Improve description for @oneOf directive (#3937)
c67dc4d
@benjie benjie mentioned this pull request Jun 21, 2024
Merged
@jasonkuhrt jasonkuhrt mentioned this pull request Oct 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaacovCR yaacovCR yaacovCR left review comments

@benjie benjie benjie approved these changes

Assignees
No one assigned
Labels
PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@spawnia @benjie @IvanGoncharov @yaacovCR