Skip to content

Split out 'ASTValidationContext' #1446

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

Merged
merged 1 commit into from
Aug 3, 2018

Conversation

IvanGoncharov
Copy link
Member

Splited from #1438

@IvanGoncharov
Copy link
Member Author

@mjmahone I decided to split it out of #1438 since it's pretty simple change.
Addressing your comment:

However, I am beginning to think there are three pieces:
- a pure "ErrorReporter". Has no AST knowledge.
- an AST-only context. This is `SDLValidationContext` as it stands. Everything that is a pure `visitor` can use this context. It may need two documents, Schema and Executable ASTs.
- a Schema-aware context. Much like `visitor` can take in a `visitWithTypeInfo`, this would hold a `ASTValidationContext`, and delegate to that one for the ast functionality.
  • a pure "ErrorReporter". Has no AST knowledge.

Do you have use case for this class? Maybe we can reuse it for validateSchema.
Anyway I think it should be extracted together with feature that make use of it.

an AST-only context.

I renamed BaseValidationContext to ASTValidationContext.

It may need two documents, Schema and Executable ASTs.

It to have context that validate basic things (e.g. unique arg names) without schema or any other additional data and can reuse both for GQL and SDL validations.

Can you please review this PR?

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.

This is great!

@mjmahone
Copy link
Contributor

mjmahone commented Aug 3, 2018

Do you have use case for this class? Maybe we can reuse it for validateSchema

Yeah, that was my thought. It's also pretty common to want to "collect but not throw errors", and then throw them all at once. Having a default error reporter provided by graphql-js would make that pattern a little bit easier to use.

It to have context that validate basic things (e.g. unique arg names) without schema or any other additional data and can reuse both for GQL and SDL validations.

Makes sense. Thanks for putting this up!

@IvanGoncharov IvanGoncharov merged commit 0a9a533 into graphql:master Aug 3, 2018
@IvanGoncharov
Copy link
Member Author

@mjmahone BTW. I figure out we should keep all these new classes and function private until 14.1.0 to have the option to change stuff (names, args, etc.)

@mjmahone
Copy link
Contributor

mjmahone commented Aug 3, 2018

That makes sense, and I agree. Thanks for thinking through it.

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.

3 participants