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

Support for GraphQL comments #4970

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

comatory
Copy link

@comatory comatory commented May 24, 2024

Resolves #4905

This PR is in draft state, we want to get feedback early so we know we're on the right track.

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:

# we added support for comments
type MyType {
  name: String!
}

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 desireable because those would leak into documentation.

The next steps are to add support for comments to every possible place according to GraphQL specification, e.g. on definitions for enums, inputs, unions etc.
At the moment, we are fine with having single-line comments and expand to multi-line comments in subsequent PRs, which means eventually we want this to be possible:

# ❌ not yet implemented
# first line
# second line
type MyType {
  name: String!
}

We haven't yet looked into edge cases such as mixing descriptions with comments, but eventually those should be supported as well.

@rmosolgo
Copy link
Owner

👍 I think this is absolutely on the right track!

Let me know if I can do anything particular to help this work along.

@comatory
Copy link
Author

@rmosolgo could you please approve CI pipeline run? Thanks

Next steps for us is to implement support for multi-line comments, we'd need to be able to parse them. For printing (output), we were thinking of for allowing newline characters if user wished to have multiline output. This would mean the comment argument could accept value such as: "first line\nsecond line" and the output would be:

# first line
# second line

We think it's good enough API for users to format it this way instead of some complex solution, such as allowing to pass lists or having some kind of automated mechanism (e.g. break after 80 chars).

If you think that printing multiple line comments is something that's not needed or wanted, we're OK with not having it.

@@ -269,6 +275,7 @@ def definition
else
loc = pos
desc = at?(:STRING) ? string_value : nil
comment = at?(:COMMENT) ? value : comment
Copy link
Author

Choose a reason for hiding this comment

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

This code is not very easy to read, to be honest we added it because one of the tests was failing and this one worked.

@rmosolgo
Copy link
Owner

I agree that working from line breaks is the right way to render multiline comments. Ideally, GraphQL-Ruby would be able to parse SDL with multiline comments and print them back out exactly as they were originally written. (For your use case with es-lint, that might even be a hard requirement -- we don't want those directives getting merged with the explanatory comments above them, right?)

@comatory
Copy link
Author

Ideally, GraphQL-Ruby would be able to parse SDL with multiline comments and print them back out exactly as they were originally written. (For your use case with es-lint, that might even be a hard requirement -- we don't want those directives getting merged with the explanatory comments above them, right?)

Yeah exactly, we should respect that during parsing that's for sure. Yes for eslint, it's important the specific comment needs to be placed right above the statement it's related to.

We'll try to polish this first and have the tests pass, which will probably require us to figure out multiline comments in the process 👍

@vaclavbohac vaclavbohac force-pushed the feat/vbo-field-comments branch 2 times, most recently from 62ea569 to 0a1135d Compare June 14, 2024 06:42
@comatory
Copy link
Author

@rmosolgo 👋 hello, is it possible to approve running pipelines on this PR? We're in the process of fixing specs and it'd be nice if we could see results in CI. 🙇

@rmosolgo
Copy link
Owner

👀 I see the button to allow CI once, but I'm not sure how enable it for a PR. I couldn't find a doc from GitHub on how to do it -- do you know how?

@comatory
Copy link
Author

@rmosolgo Hm I thought there might be some settings in repository. I only found this but not sure if it helps. Anyway, thanks!

@rmosolgo
Copy link
Owner

Ah... yes, there is a setting there: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#controlling-changes-from-forks-to-workflows-in-public-repositories

I switched it so only "new GitHub accounts" need me to approve them. Hopefully that will do it 🤞

@rmosolgo rmosolgo closed this Jun 14, 2024
@rmosolgo
Copy link
Owner

Wrong button 🤦

@vaclavbohac vaclavbohac force-pushed the feat/vbo-field-comments branch 2 times, most recently from 75eca02 to 7a4ddf2 Compare July 2, 2024 12:06
@vaclavbohac
Copy link

@rmosolgo Hello, we've got to a point where most of the tests are passing (of course the changes themselves are far from final) but we are stuck on the c-parser integration. It's a little mysterious how it works to us and we could use some guidance how to fix it and what the best approach here would be. Could you help us please?

cc @comatory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possibility to generate GraphQL comments
3 participants