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

Add links to contributed custom scalar specs at scalars.graphql.org #1009

Merged
merged 7 commits into from
Feb 11, 2023
Merged

Add links to contributed custom scalar specs at scalars.graphql.org #1009

merged 7 commits into from
Feb 11, 2023

Conversation

dondonz
Copy link
Contributor

@dondonz dondonz commented Jan 15, 2023

The PR follows on from the launch of scalars.graphql.org.
https://github.com/graphql/graphql-scalars
https://graphql.org/blog/2023-01-14-graphql-scalars/

As discussed in the January APAC working group meeting, this PR adds how to contribute custom scalar specs, links to spec templates, and adds examples.

cc @andimarek

@netlify
Copy link

netlify bot commented Jan 15, 2023 •
edited
Loading

✅ Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 3671a96
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/63cf0f4f194a7b00088e28aa
😎 Deploy Preview https://deploy-preview-1009--graphql-spec-draft.netlify.app/draft
📱 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 settings.

@dondonz
Copy link
Contributor Author

dondonz commented Jan 15, 2023

Feel free to comment on the wording

@andimarek
Copy link
Contributor

I am not 100% regarding the process here: I don't see this as the normal trivial editorial change (e.g. typo), but it is also no real change in the spec. Therefore I am leaning to treat it as editorial change.

Keen to hear what others think @benjie @michaelstaib @leebyron

benjie
benjie requested changes Jan 16, 2023
View reviewed changes
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Suggest we shrink this PR somewhat and ensure the changes are in non-normative notes. I've included some suggestions, but you might not want to use them verbatim - edit as you see fit.

spec/Section 3 -- Type System.md Outdated Show resolved Hide resolved
spec/Section 3 -- Type System.md Outdated Show resolved Hide resolved
spec/Section 3 -- Type System.md Outdated Show resolved Hide resolved
spec/Section 3 -- Type System.md Outdated Show resolved Hide resolved
dondonz and others added 4 commits January 17, 2023 10:07
@dondonz @benjie
Update spec/Section 3 -- Type System.md
c822e9c 
Co-authored-by: Benjie <benjie@jemjie.com>
@dondonz
Remove extra link
f6bce48
@dondonz @benjie
Update spec/Section 3 -- Type System.md
bb7acf0 
Co-authored-by: Benjie <benjie@jemjie.com>
@dondonz @benjie
Update spec/Section 3 -- Type System.md
f190d21 
Co-authored-by: Benjie <benjie@jemjie.com>
@dondonz
Copy link
Contributor Author

dondonz commented Jan 16, 2023

Thanks @benjie ! I've put through all your comments

@dondonz dondonz requested a review from benjie January 16, 2023 23:16
andimarek
andimarek reviewed Jan 22, 2023
View reviewed changes
spec/Section 3 -- Type System.md Outdated
In this example, a custom scalar type for `UUID` is defined with a URL pointing
to the relevant IETF specification.

```graphql example
scalar UUID @specifiedBy(url: "https://tools.ietf.org/html/rfc4122")
scalar URL @specifiedBy(url: "https://tools.ietf.org/html/rfc3986")
Copy link
Contributor

Choose a reason for hiding this comment

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

I honestly think these are not good examples: we should not encourage people to link directly
to RFCs. I would even suggest to remove the UUID example. A RFC is great to be the basis of a custom scalar, but it is almost never sufficient to be a good guide. We should people actively encourage to write dedicated custom scalars specs.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed; writing a scalars.graphql.org spec that calls out to the relevant RFC seems wise. I'm not sure if that needs to be done in this PR though?

Copy link
Member

Choose a reason for hiding this comment

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

It definitely doesn't need to be addressed in this PR; it's already referenced on line 407, this is just a copy. Can someone file it for follow-up though?

benjie
benjie requested changes Jan 23, 2023
View reviewed changes
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Looking good, let's trim those final two lines and then I think we're good to go (after you run npm install && npm run format to appease Prettier) 👍

spec/Section 3 -- Type System.md Outdated Show resolved Hide resolved
dondonz and others added 2 commits January 24, 2023 09:40
@dondonz @benjie
Update spec/Section 3 -- Type System.md
b81953a 
Co-authored-by: Benjie <benjie@jemjie.com>
@dondonz
Appease Prettier
3671a96
@dondonz
Copy link
Contributor Author

dondonz commented Jan 23, 2023

I have spoken the magic words npm install && npm run format and I hope Prettier is pleased.

I'm ready for a final review @benjie

@dondonz
Copy link
Contributor Author

dondonz commented Feb 1, 2023

Hi @benjie could you please have another review?

benjie
benjie approved these changes Feb 10, 2023
View reviewed changes
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Looks good to me; thanks @dondonz! I'd like another approval from @graphql/tsc and then I think we're good to merge 👍

mjmahone
mjmahone approved these changes Feb 10, 2023
View reviewed changes
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.

These changes seem clean to me, all in favor of shipping!

@leebyron leebyron added the ✏️ Editorial PR is non-normative or does not influence implementation label Feb 11, 2023
@leebyron
Copy link
Collaborator

Nice work! Glad to have these references in place

@leebyron leebyron merged commit afc0a35 into graphql:main Feb 11, 2023
@dondonz dondonz deleted the custom-scalar-specs branch February 11, 2023 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andimarek andimarek andimarek left review comments

@benjie benjie benjie approved these changes

@mjmahone mjmahone mjmahone approved these changes

Assignees
No one assigned
Labels
✏️ Editorial PR is non-normative or does not influence implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dondonz @andimarek @leebyron @benjie @mjmahone