Schema rename suppress types #834
Conversation
|
Summary:
Not sure what's going on with the failing CI tests-- is this a known issue? Doesn't seem like anything in this PR would prevent them from running and the internet hasn't brought up much yet. |
There was a problem hiding this comment.
Lots of good progress here! There is lots of complexity and many non-obvious edge cases to sort out, and it is totally expected that we'll need a few iterations to converge on the best approach — so don't worry about the number of comments. Most of them are suggestions around improving end user ergonomics, and for things to watch out for that will make future code reviews faster and easier for both you and reviewers.
| # then this field depends on a type that was suppressed, which is illegal | ||
| type_name = args[-1][-1].name.value | ||
| raise CascadingSuppressionError( | ||
| f"Type renamings {self.renamings} attempted to suppress a type, but type " |
There was a problem hiding this comment.
This seems like it would raise an error separately for each field it runs into, one at a time. That means fixing everything wrong with a schema requires many iterations where the code complains about one wrong thing at a time, instead of giving you everything that is wrong all at once. So this is working code, but not particularly ergonomic from an end-user point of view. See my comment below for an alternative suggestion that would make raising an error about all the issues at once easier.
Mostly type-hints, nits
Still a few linting errors that I'm not sure about, will look at tomorrow
I know these two aren't the same, technically (python/mypy#3138) but the original code listed them as Any and not Optional[Any] so just type-hinting Any here is more accurate.
| self.query_type = query_type | ||
| self.scalar_types = frozenset(scalar_types) | ||
| self.builtin_types = frozenset({"String", "Int", "Float", "Boolean", "ID"}) | ||
|
|
||
| def _rename_name_and_add_to_record(self, node): | ||
| def _rename_name_and_add_to_record(self, node: Node) -> Union[type(REMOVE), Optional[Node]]: |
There was a problem hiding this comment.
This is the other lint error we're seeing, which is because the enter function has node type as Node, but this function will only be called with a node of type in self.rename_types, and all of those nodes have names. Not sure how to write that in the type-hint though-- putting anything except Node as the type-hint makes mypy complain in the enter function that calls _rename_name_and_add_to_record
edit: i may have figured out a workaround involving cast
There was a problem hiding this comment.
I've confirmed that the following workaround makes the linter happy, but is quite hacky:
Add type_hint_rename_types to RenameSchemaTypesVisitor as another class attribute (parallels the rename_types frozenset that already exists)
type_hint_rename_types = Union[
EnumTypeDefinitionNode,
InterfaceTypeDefinitionNode,
NamedTypeNode,
ObjectTypeDefinitionNode,
UnionTypeDefinitionNode,
]
Then we can use this Union as the type in the function signature for _rename_name_and_add_to_record and replace the call self._rename_name_and_add_to_record(node) with
renamed_node = self._rename_name_and_add_to_record(
cast(RenameSchemaTypesVisitor.type_hint_rename_types, node)
)
Trying to turn the existing set to a mypy Union doesn't work syntactically (e.g. iterating over these 5 types don't work) as far as I can tell, but I'd be very happy to be proven wrong!
|
|
||
| Args: | ||
| ast: Document, representing a valid schema that does not contain extensions, input | ||
| object definitions, mutations, or subscriptions, whose fields of the query type share | ||
| the same name as the types they query. Not modified by this function | ||
| renamings: Dict[str, str], mapping original type/field names to renamed type/field names. | ||
| Type or query type field names that do not appear in the dict will be unchanged. | ||
| renamings: Dict[str, Optional[str]], mapping original type/root type field names to renamed |
There was a problem hiding this comment.
Consider removing the types from the Args section. Now that we have typehints, adding the type here is redundant and could accidentally get out of date.
There was a problem hiding this comment.
Sounds good-- this PR is already pretty big so after this gets merged I'll be removing these in a separate PR
There was a problem hiding this comment.
At this point, I think this PR is doing too many things and needs to be split up before it can be merged.
I'd suggest the following:
- one or more PRs that simply add type hints and associated code without making any functional changes whatsoever
- one or more PRs that then only add new functionality, without changing unrelated code
|
That makes sense-- since this'll break it into several smaller PRs, I'll also be able to incorporate some of the comments that I hadn't touched previously. Here's how I plan to split it up:
|
|
Sounds great! |
Described [here](https://github.com/kensho-technologies/graphql-compiler/pull/834/files#r432563284) and [here](https://github.com/kensho-technologies/graphql-compiler/pull/834/files#r432562122). Since I'm going to split this PR into several parts instead of trying to merge all the code here, I didn't polish this commit super carefully-- just a draft of a way to do this and making sure I don't lose this code.
Turns out that checking for CascadingSuppressionError before any types get modified doesn't require modifying the AST at all.
* Type-hint existing function signatures * Remove types from function comments * Fix type-hint problem requiring name attribute See #834 (comment) * lint * rename ast to schema_ast * arg/return value definitions to lowercase * Remove leading articles for parameter descriptions * Dynamically confirm type-hint and rename_types contain the same information * rename type_hint_rename_types to RenameTypes * lint * compute rename_types dynamically * lint * Decide on duplication as cleanest solution * Replace Dict with Mapping for renaming type-hint * Tighten return type bound * Bump linter version to prevent false positive error * Revert "Bump linter version to prevent false positive error" This reverts commit 7579db6. * Make RenameTypes a module-level attribute instead of a class attribute * Add Generic type * lowercase first word of return description Co-authored-by: Predrag Gruevski <[email protected]> * Switch Set to AbstractSet type hint * remove newline * Add newline after multiple de-indent Co-authored-by: Predrag Gruevski <[email protected]> Co-authored-by: Predrag Gruevski <[email protected]>
|
Now that PR 858 and 859 are out in draft form, all the significant changes described here have either already been merged or exist in one of those two PRs so I'll close this PR and delete this branch when we're done. For the time being, though, I'll keep this open until those two get merged just in case there's something here that I've missed by mistake. |
|
Converting to draft because PRs 858 and 859 (plus a bit more work with enums and interfaces) will supersede the work here. update: Almost all the work in this PR has been merged into main (except interface-related tests which require more attention), so I'm closing this PR |
First part of the implementation described in pull request #828 and issue #810, covering just 1-0 renaming for types (aka suppressing types). I'm going to read over the changes here first and then when it's ready, I'll mark it as ready for review