Add SchemaVisitor and QueryVisitor traits and functions #26
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 |
|
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); | ||
| } |
tomhoule
Sep 20, 2019
•
Member
Would it make sense to further walk down the fragment's selection?
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); | ||
| } |
tomhoule
Sep 20, 2019
Member
Same as for fragments, would it make sense to visit the selection set?
Same as for fragments, would it make sense to visit the selection set?
|
This looks great to me. |
|
@tailhook will leave it to you to look at and land. |
|
It looks like there are multiple mistakes with visiting recursively. I'm not sure how to structure code or how to test it well to eliminate these bugs. (sorry that it took so long...) |
| visitor.visit_field(inner); | ||
| walk_field(visitor, inner); |
tailhook
Apr 1, 2020
Collaborator
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?
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?
| //! | ||
| //! walk_document(&mut number_of_type, &doc); | ||
| //! | ||
| //! assert_eq!(number_of_type.count, 2); |
tailhook
Apr 1, 2020
Collaborator
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.
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(); |
spawnia
Apr 10, 2020
•
Suggested change
//! 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.
| //! 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.
Fixes #25
Implementation is based upon the description here https://github.com/rust-unofficial/patterns/blob/master/patterns/visitor.md