Improve type checking in query pagination and AST manipulation code. #898
Conversation
Includes type-level fixes for handling InlineFragmentNode AST values in query pagination code, a couple of new helper functions, and type hints for the AST manipulation module.
obi1kenobi
added
enhancement
maintainer quality-of-life
Codecov Report
@@ Coverage Diff @@
## main #898 +/- ##
==========================================
- Coverage 95.07% 94.98% -0.10%
==========================================
Files 111 111
Lines 8788 8767 -21
==========================================
- Hits 8355 8327 -28
- Misses 433 440 +7
Continue to review full report at Codecov.
|
bojanserafimov
left a comment
bojanserafimov
left a comment
There was a problem hiding this comment.
Looks good. We can take the inline fragment discussion to #897
I have a nit.
graphql_compiler/ast_manipulation.py
Outdated
| return ast | ||
|
|
||
|
|
||
| def assert_selection_is_a_field_node(ast: Any) -> FieldNode: |
There was a problem hiding this comment.
It's strange for an assert function to return, though I get why it has to.
In relation to mypy's cast function, this is just a safe_cast. If you don't find mypy's use of the word cast repulsive, this would be a good name for this function. We can even make it generic: safe_cast(value: Any, type: Type[T]) -> T.
I personally don't like mypy's naming of cast since there's no change of shape involved (like in metal casting). It should have been called unsafe_assume_type. But the function exists, there's nothing I can do about it, and calling our function safe_cast makes it a discoverable alternative to cast.
There was a problem hiding this comment.
I like the idea — perhaps checked_cast as an alternative name, to make the answer to "how is this safer" a bit more explicit?
There was a problem hiding this comment.
checked_cast() doesn't work with Union because Union is not a Type, so Type[T] doesn't match.
For bounded-length tuples, this is achievable:
def checked_cast_to_union2(target_types: Tuple[Type[T_A], Type[T_B]], value: Any) -> Union[T_A, T_B]:
if not isinstance(value, target_types):
raise AssertionError()
return valueI don't know of a way to make target_types: Tuple[Type, ...] work and connect the types it contains to the types inside the Union result.
Given that we're somewhat unlikely to have unions of more than like 3 or so things, I'm fine with adding checked_cast_to_union2() and in the future potentially adding a checked_cast_to_union3() function that takes a 3-tuple as necessary.
What do you think?
| # N.B.: We cannot use assert_selection_is_a_field_or_inline_fragment_node() here, since | ||
| # that function's return type annotation is a Union type that isn't compatible | ||
| # with the generic type signature of _add_pagination_filter_at_node(). |
There was a problem hiding this comment.
Interesting how assert_selection_is_a_field_or_inline_fragment_node works with unions: "We used to know x is of type A, but we ran some assertions and now we're not sure if it's A or B"
Can't think of a cleaner solution.
Btw, we use N.B. and NOTE in the codebase. No need to standardize, but letting you know that NOTE gets highlighted by many text editors, just like TODO is.
There was a problem hiding this comment.
If checked_cast() works out, we might not need this note anymore. 🤞
There was a problem hiding this comment.
I wrote a checked_cast_to_union2() function but it turns out that even it can't do the trick with generics. I updated the note with my new observations, let me know if you want to talk about it.
|
@bojanserafimov I'd love a re-review here when you get the chance. |
Includes type-level fixes for handling
InlineFragmentNodeAST values in query pagination code, a couple of new helper functions, and type hints for the AST manipulation module.Based on the tests passing and my careful refactoring, I am fairly confident that I haven't made pagination any worse. However, I opened #897 to verify that query pagination in the presence of type coercions (signaled by
InlineFragmentNodeAST values) actually works.This is quite a tricky PR, so I'd like to have @bojanserafimov review it before we merge, in addition to anyone else that might want to check it out.