Add SchemaVisitor and QueryVisitor traits and functions#26
Add SchemaVisitor and QueryVisitor traits and functions#26davidpdrsn wants to merge 4 commits intographql-rust:masterfrom
Conversation
|
It looks good to me on the whole! It's unsollicited, but if nobody else has the time, I could do a more in-depth review. |
|
@tomhoule that would be great 😊 |
tomhoule
left a comment
tomhoule
left a comment
There was a problem hiding this comment.
I'm not the maintainer of this crate, but as far as I'm concerned it looks good :)
|
|
||
| fn walk_fragment_definition<'ast, V: QueryVisitor<'ast>>(visitor: &mut V, node: &'ast FragmentDefinition) { | ||
| visitor.visit_fragment_definition(node); | ||
| } |
There was a problem hiding this comment.
Would it make sense to further walk down the fragment's selection?
|
|
||
| fn walk_inline_fragment<'ast, V: QueryVisitor<'ast>>(visitor: &mut V, node: &'ast InlineFragment) { | ||
| visitor.visit_inline_fragment(node); | ||
| } |
There was a problem hiding this comment.
Same as for fragments, would it make sense to visit the selection set?
|
@tailhook will leave it to you to look at and land. |
| visitor.visit_field(inner); | ||
| walk_field(visitor, inner); |
There was a problem hiding this comment.
I don't understand this pattern. You first do visit_field and then walk_field which calls visit_field again. Same with fragments. Am I misunderstanding something?
There was a problem hiding this comment.
I have a similar question. It seems that every walk function calls visit_node and then then calls a walk_node function that calls the same visit_node. For example walk_subscription calls visitor.visit_selection_set and then calls walk_selection_set which in turn calls visitor.visit_selection_set a second time.
Is that how it's supposed to work? I thought you only need to visit once.
| //! | ||
| //! walk_document(&mut number_of_type, &doc); | ||
| //! | ||
| //! assert_eq!(number_of_type.count, 2); |
There was a problem hiding this comment.
This doesn't look right. There are four fields if visiting recursively: query, id, country, id and single one if not (users). Currently it looks like you just counting users twice.
Or was non-recursive visitor intentional? |
| //! } | ||
| //! | ||
| //! fn main() { | ||
| //! let mut number_of_type = FieldsCounter::new(); |
There was a problem hiding this comment.
| //! let mut number_of_type = FieldsCounter::new(); | |
| //! let mut fields_counter = FieldsCounter::new(); |
number_of_type seems to indicate this is supposed to count types, not fields, which does not really make sense when walking over a query.
|
Hi y'all, I'm gonna try another experimental flavor of an implementation for this, following the
The biggest use-case of a system like this in my case is doing custom validation of a query based on a schema, this API lets you do that. |
|
We implemented a visitor (with type info) for both schema and queries in https://github.com/dotansimha/graphql-tools-rs , also a (almost) spec-compliant validation phase. Thanks for this PR, it was a great inspiration! |
|
@dotansimha why did you post this in 3 different places? 😅 |
I saw these PRs are duplicated anyway, so I hope it would help someone :) |
|
I'll close this for now. Its been a long time and I no longer have bandwidth to follow this through. |
7 participants
Fixes #25
Implementation is based upon the description here https://github.com/rust-unofficial/patterns/blob/master/patterns/visitor.md