Remove definition iteration from 'ExecutableDefinitions' rule #1466
Conversation
| @@ -27,23 +30,24 @@ export function ExecutableDefinitions( | |||
| context: ASTValidationContext, | |||
| ): ASTVisitor { | |||
| return { | |||
| Document(node) { | |||
| for (const definition of node.definitions) { | |||
| if (!isExecutableDefinitionNode(definition)) { | |||
There was a problem hiding this comment.
@mjmahone I was searching for non-SDL examples of iterating over all definition pattern and found this 😄
IvanGoncharov
force-pushed
the
simplifyExecutableDefinitionsRule
branch
from
f402403 to
71759eb
Compare
| for (const definition of node.definitions) { | ||
| if (!isExecutableDefinitionNode(definition)) { | ||
| enter(node) { | ||
| if (isDefinitionNode(node)) { |
There was a problem hiding this comment.
We can combine these into a single if statement:
if (isDefinitionNode(node) && !isExecutableDefinitionNode(node)) {
There was a problem hiding this comment.
Actually no, since you need to return false if isDefinitionNode is true to skip other definitions.
But I starting to think this solution is too complicated and we need to find a generic one for all similar cases.
There was a problem hiding this comment.
Ah you're right, I missed the return false
There was a problem hiding this comment.
I don't think this is too complex as-is. And I wouldn't want to block this clean up on figuring out how to abstract this idea.
I do think we should provide an "enum type" for the return values of the ASTVisitor, like
const STOP_TRAVERSAL = false;
...
type VISITOR_RETURN_VALUEs =
| STOP_TRAVERSAL
| ...;
But that's mostly unrelated to this.
| for (const definition of node.definitions) { | ||
| if (!isExecutableDefinitionNode(definition)) { | ||
| enter(node) { | ||
| if (isDefinitionNode(node)) { |
There was a problem hiding this comment.
Ah you're right, I missed the return false
No description provided.