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

RFC: Descriptions as strings. #927

Merged
merged 1 commit into from
Nov 30, 2017
Merged

RFC: Descriptions as strings. #927

merged 1 commit into from
Nov 30, 2017

Conversation

leebyron
Copy link
Contributor

@leebyron leebyron commented Jun 22, 2017

Breaking Change - those using comment descriptions will need to provide {commentDescriptions: true} to the relevant functions

As discussed in graphql/graphql-spec#90

This proposes replacing leading comment blocks as descriptions in the schema definition language with leading strings.

While I think there is some reduced ergonomics of using a string literal instead of a comment to write descriptions (unless perhaps you are accustomed to Python or Clojure), there are some compelling advantages:

  • Descriptions are first-class in the AST of the schema definition language.
  • Comments can remain "ignored" characters.
  • No ambiguity between commented out regions and descriptions.

Specific to this reference implementation, since this is a breaking change and comment descriptions in the experimental SDL have fairly wide usage, I've left the comment description implementation intact and allow it to be enabled via an option. This should help with allowing upgrading with minimal impact on existing codebases and aid in automated transforms.

Note: This PR is dependent on #926 and should not be merged out of order.

@rmosolgo
Copy link

Coming from Ruby, comments-as-docs felt very familiar. (In Ruby, comments are all we've got 😆) Folks have been happily using that approach in graphql-ruby for quite a while.

some reduced ergonomics of using a string literal

It seems like the goal here is to simplify the AST, even if it comes at the cost of usability, is that right?

If that's true, what are the benefits of a simpler AST, and who benefits from it? (It was tricky to implement comments-as-descriptions, but now that it's there, we haven't had any trouble with them!)

No ambiguity between commented out regions and descriptions.

I guess this could happen if you commented out one field, right about the other?

type Thing {
  # id: ID!           <- oops?
  name: String!
}

Or are there other spots were people were tripped up by commenting out code?

@stubailo
Copy link

stubailo commented Jul 3, 2017

I like this a lot precisely for the reasons you mentioned, @leebyron - I think it's an elegant way of not overloading comments to do multiple totally different things, while not expanding the language syntax with totally new symbols or concepts.

As discussed in graphql/graphql-spec#90

This proposes replacing leading comment blocks as descriptions in the schema definition language with leading strings (typically block strings).

While I think there is some reduced ergonomics of using a string literal instead of a comment to write descriptions (unless perhaps you are accustomed to Python or Clojure), there are some compelling advantages:

* Descriptions are first-class in the AST of the schema definition language.
* Comments can remain "ignored" characters.
* No ambiguity between commented out regions and descriptions.

Specific to this reference implementation, since this is a breaking change and comment descriptions in the experimental SDL have fairly wide usage, I've left the comment description implementation intact and allow it to be enabled via an option. This should help with allowing upgrading with minimal impact on existing codebases and aid in automated transforms.
@leebyron leebyron merged commit 37717b8 into master Nov 30, 2017
@leebyron leebyron deleted the rfc-docstring branch November 30, 2017 20:47
@stubailo
Copy link

stubailo commented Dec 1, 2017

@leebyron do you know when this change will be published? That will be a big breaking change for graphql-tools users since a lot of people are using comments as descriptions, so I'd like to be on top of it.

kumarharsh pushed a commit to kumarharsh/graphql-for-vscode that referenced this pull request Jan 6, 2018
- graphql v0.12.0 has a new proposed way of adding field descriptions as strings rather than comments. See graphql/graphql-js#927. This commit adds support for the usecase.
mattleff added a commit to mattleff/DefinitelyTyped that referenced this pull request Jan 25, 2018
These were added with graphql/graphql-js#927, RFC: Descriptions as strings.
ghost pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Jan 25, 2018
* Add schema printer options

These were added with graphql/graphql-js#927, RFC: Descriptions as strings.

* Explicitly define allowed printer options
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.

4 participants