Refactor cascading validation #995
Conversation
| @@ -235,7 +235,7 @@ def rename_schema( | |||
| schema = build_ast_schema(schema_ast) | |||
| query_type = get_query_type_name(schema) | |||
|
|
|||
| _validate_renamings(schema_ast, type_renamings, field_renamings, query_type) | |||
| _ensure_no_unsupported_suppressions(schema_ast, type_renamings) | |||
There was a problem hiding this comment.
After this PR, the only separate validation step left would be this one, so there's no need for a special _validate_renamings function.
Codecov Report
@@ Coverage Diff @@
## main #995 +/- ##
==========================================
+ Coverage 94.33% 94.38% +0.05%
==========================================
Files 113 113
Lines 9070 9080 +10
==========================================
+ Hits 8556 8570 +14
+ Misses 514 510 -4
Continue to review full report at Codecov.
|
| type_name = node.name.value | ||
| current_type_fields_to_suppress = {} | ||
| for field_node in node.fields: |
There was a problem hiding this comment.
This loop strongly resembles the one below on line 806, but I'm not sure of how to organize it best. The problem here is with the function structure. Let's say we're at a type T with a field foo, and foo is of type Bar`. Then this function is essentially doing the following:
- For each field
fooof typeBar: Is the typeBargoing to be suppressed? If so, and iffooisn't going to be suppressed, the renamings are invalid and we should do the bare minimum necessary to continue the schema renaming and collect any other independent errors while recording the problem. Return from this node-- no more to be done here. - Do we expect to renaming
T's fields at all? If not, we're done and can return. - For each field
foo: renamefooif necessary.
I considered trying something along the lines of implementing step 2 via "if T's fields are not to be renamed, this is equivalent to using the identity mapping for all of T's fields with the reasoning that this means the loops in steps 1 and 3 can probably combined into one single loop, but it's not clear to me that this is necessarily easier to understand. Would definitely invite any feedback!
There was a problem hiding this comment.
This PR comment would be useful to integrate into the code itself. The logic around line 780 is quite complex and it's not really obvious from the source code itself what is being done and why.
In particular, I'd love to see complex expressions like self.type_renamings.get(field_type_name, field_type_name) is None get assigned to well-named local variables, because they are complex and surprising — in this case, that's because it's unusual to see a .get() check with an explicit non-None default value be checked against a None result.
What I'd recommend is writing something similar to your PR comment as a code comment in a continuous block just above this for loop, and then reference the individual parts of the algorithm the comment describes from the appropriate place inside the for loop. Together with good naming of intermediate expression values, I think this code will become much clearer.
| type_name = node.name.value | ||
| current_type_fields_to_suppress = {} | ||
| for field_node in node.fields: |
There was a problem hiding this comment.
This PR comment would be useful to integrate into the code itself. The logic around line 780 is quite complex and it's not really obvious from the source code itself what is being done and why.
In particular, I'd love to see complex expressions like self.type_renamings.get(field_type_name, field_type_name) is None get assigned to well-named local variables, because they are complex and surprising — in this case, that's because it's unusual to see a .get() check with an explicit non-None default value be checked against a None result.
What I'd recommend is writing something similar to your PR comment as a code comment in a continuous block just above this for loop, and then reference the individual parts of the algorithm the comment describes from the appropriate place inside the for loop. Together with good naming of intermediate expression values, I think this code will become much clearer.
| path: List[Any], | ||
| ancestors: List[Any], | ||
| ) -> None: | ||
| """Check that each union still has at least one non-suppressed member.""" |
There was a problem hiding this comment.
nit: This docstring doesn't really capture what's going on. The implementation does an action if that check fails, but the docstring doesn't make any mention of that.
Consider instead:
| """Check that each union still has at least one non-suppressed member.""" | |
| """Suppress unions that do not have any remaining non-suppressed members.""" |
There was a problem hiding this comment.
The action that happens if the check fails is actually raising an error instead of automatically suppressing the union (since renaming is defined so that unions in such situations have to get explicitly suppressed) and the error handling code is outside of the visitor itself, so I'm inclined to keep the docstring as-is. Worth adding a bit to mention what the union_types_to_suppress field is going to be used for, even if it doesn't happen in this function itself?
There was a problem hiding this comment.
It isn't clear to me whether you were done tweaking this PR — when you're done, hit that "re-request review" button next to my name in the reviewers list to put this PR back on my review queue.
I have two minor suggestions, and otherwise this seems fine to merge.
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
As described in PR #994:
This PR continues cleaning up this technical debt. It takes cascading suppression errors and moves the checks into the renaming code rather than having a separate validation step, simplifying the renaming process.
This PR lets us remove the entire
CascadingSuppressionCheckVisitorclass. If it'd be significantly easier to understand the changes here if I tried moving the functionality out of theCascadingSuppressionCheckVisitorpiecemeal, just let me know and I can do that-- here I decided not to do that initially since all the code in the visitor follows the same pattern of "fooin the schema depends onbarwherebaris to be suppressed".