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

lexicographic_sort_schema is underspecified. #75

Closed
pmantica1 opened this issue Jan 22, 2020 · 8 comments
Closed

lexicographic_sort_schema is underspecified. #75

pmantica1 opened this issue Jan 22, 2020 · 8 comments

Comments

@pmantica1
Copy link

pmantica1 commented Jan 22, 2020

First of all thanks for the amazing work in porting the GraphQL reference implementation.

I wanted to compute the fingerprint of a schema by running lexicographic_sort_schema and print_schema then computing the sha256 of the output.
kensho-technologies/graphql-compiler#737

However, it is unclear whether lexicographic_sort_schema modifies the input schema. So I was having trouble documenting my function. Would you mind clarifying the function's spec?

@pmantica1 pmantica1 changed the title lexicographic_sort_schema is underspecified. lexicographic_sort_schema is underspecified. Jan 22, 2020
@Cito
Copy link
Member

Cito commented Jan 23, 2020

The function takes a schema and returns a new, sorted schema. The input schema should not be modified (otherwise, please file a bug report).

@obi1kenobi
Copy link

obi1kenobi commented Jan 23, 2020

I suspect that "parse schema -> sort -> print schema" may be a common usage pattern of lexicographic_sort_schema. There may be value in us documenting some of the sharp edges that @pmantica1 uncovered in this pattern, even though lexicographic_sort_schema is not the cause, purely as a nota bene to future users. A couple of examples:

If you think this is a good idea, @pmantica1 and I can work on a PR adding these notes to the lexicographic_sort_schema docstring.

Thanks again for all the work you've put into this library! Using it has been a pleasure, and I'd love to help in any way I can.

@Cito
Copy link
Member

Cito commented Jan 24, 2020

Adding more documentation can never harm. If you think something needs to be improved or discussed more fundamentally, maybe it's a good idea to create a ticket at GraphQL.js. The way how the functions work and the docsstrings are ported from there and it's always best to fix things at the source.

@obi1kenobi
Copy link

Thanks! I'll start by opening a ticket in GraphQL.js and see how people feel about the idea of parse-print producing SDL equivalent to the original input, then based on the feedback either write some nota bene documentation or pitch in for any code changes that may be necessary.

@Cito
Copy link
Member

Cito commented Jan 24, 2020

Just saw Ivan answered this has already been improved in GraphQL.js 15.0.0-alpha.2. In fact the current master in GraphQL core only has the changes up to 15.0.0-alpha.1. These improvements will probably be ported in the next weeks as I am following up.

@obi1kenobi
Copy link

graphql/graphql-js#2389 appears to still be a concern in GraphQL.js 15.0.0-alpha.2.

Also, @pmantica1 and I are happy to pitch in and open PRs to port the GraphQL.js fixes if you'd like any help.

@Cito
Copy link
Member

Cito commented Jan 25, 2020

Thank you. I'll go over the new changes in the coming days and let you know if you can help with anything. I'm trying to port every change in order, so this needs some coordination.

@Cito
Copy link
Member

Cito commented Feb 10, 2020

Just a small progress report: GraphQL-Core v3.1.0b0 has been released today which is on par with GraphQL.js v15.0.0rc1. But as far as I understand, the problem is not yet fully solved upstream.

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

No branches or pull requests

3 participants