-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support for comments v2 #5067
Support for comments v2 #5067
Conversation
9116887
to
d4228b0
Compare
3cd958a
to
7cacd89
Compare
Hello, @rmosolgo finally I think the implementation is ready for review. There are some tests failing, but it seems it's rather flakiness then our changes (not 100% sure, let me know). I updated some docs as well, not sure if it's good enough. |
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 for taking another crack at this! It looks really good so far. I had a couple of questions and suggestions.
(And yes, sorry about the flaky tests ... the rails_master
test is especially flaky right now and I can't figure out why 😖 )
e542377
to
a0576b7
Compare
I made a few changes over in #5077 - I added some debugging info to the failing tests around object shapes. I think something about the implementation of I also changed the implementation so that comments are not inherited. Thinking about it, if you annotate with a parent type with some kind of linter hint, I don't think the child type should have the same hint, unless it's manually configured. Also, I updated Do those changes look good to you? (Sorry, apparently I didn't accept the invitation to productboard/graphql-ruby in time so I can't join it. You can bring that work over to this branch, or I'll merge that PR instead of this one, either way suits me.) |
@rmosolgo I cherry picked your commits, looking great. Thank you! |
Awesome. Thanks again for all your work to make this happen! |
@rmosolgo Is there a chance you could release Thank you! 🤟 |
Oh, yes, I just released it! Thanks again for this improvement 👍 |
Resolves #4905
Reimplemented #4970
The idea is that we expanded methods that construct the schema with new comment parameter (we should cover most of them like types, unions, enums). By passing string value to this new parameter, the resulting schema will contain comments such as this:
The reasons for adding this are mentioned in the linked issue but in a nutshell, we want to leverage comments for tooling (eslint), using descriptions is not desirable because those would leak into documentation.
Comments are supported for the following types: