Skip to content

validation suggestions#12

Merged
JoviDeCroock merged 2 commits intoJoviDeCroock:fragment-arguments-validation-2024from
yaacovCR:validation-suggestions
Aug 28, 2024
Merged

validation suggestions#12
JoviDeCroock merged 2 commits intoJoviDeCroock:fragment-arguments-validation-2024from
yaacovCR:validation-suggestions

Conversation

@yaacovCR
Copy link
Copy Markdown
Collaborator

@yaacovCR yaacovCR commented Aug 27, 2024

Changes:

  1. Removes NoUnusedFragmentVariables as that has been folded into NoUnusedVariables. A fragment that defines a variable must use that variable within the fragment, just like an operation that defines a variable must use it within the operation.
  2. Adds NoUnusedFragmentArguments. For example, fragments that defined nullable arguments could get away with having a document that never makes use of them. This new rule disallows that. Includes tests.
  3. Add tests for KnownArgumentNames for fragment arguments.
  4. Fixes errors for ProvidedRequiredArguments for fragment arguments
  5. Relies on the GraphQLVariableSignature type from execution (although there are slight differences that need to be reconciled)
  6. Modifies TypeInfo to allow for passing in fragment signatures when visiting a node that is not a document. This allows us to not reuse the private _typeInfo when calling visitWithTypeInfo from a ValidationContext.

Brings OperationSignatures into the ValidationContext; this could be perhaps used more?

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 27, 2024

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

JoviDeCroock
JoviDeCroock approved these changes Aug 28, 2024
View reviewed changes
Copy link
Copy Markdown
Owner

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make the spec adjustments over the weekend, I hope

Comment thread src/validation/rules/NoUnusedFragmentArgumentsRule.ts
Copy link
Copy Markdown
Owner

@JoviDeCroock JoviDeCroock Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would need reflecting in the spec but sounds good to me

Comment thread src/validation/rules/NoUnusedFragmentVariablesRule.ts
Copy link
Copy Markdown
Owner

@JoviDeCroock JoviDeCroock Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would need reflecting in the spec but sounds good to me

Comment thread src/validation/rules/NoUnusedVariablesRule.ts
Copy link
Copy Markdown
Owner

@JoviDeCroock JoviDeCroock Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would need to expand the spec

@yaacovCR
remove OperationSignature from ValidationContext
f1be0a5 
only used in one rule, does not need to be on context
@yaacovCR
Copy link
Copy Markdown
Collaborator Author

yaacovCR commented Aug 28, 2024

I removed the OperationSignature bit, wasn't adding much, reduces diff.

@yaacovCR yaacovCR mentioned this pull request Aug 28, 2024
Merged
@JoviDeCroock JoviDeCroock merged commit a53a26a into JoviDeCroock:fragment-arguments-validation-2024 Aug 28, 2024
JoviDeCroock pushed a commit that referenced this pull request Aug 29, 2024
@yaacovCR @JoviDeCroock
validation suggestions (#12)
2fb1bd5 
* validation suggestions

* remove OperationSignature from ValidationContext

only used in one rule, does not need to be on context
JoviDeCroock pushed a commit that referenced this pull request Aug 29, 2024
@yaacovCR @JoviDeCroock
validation suggestions (#12)
e416081 
* validation suggestions

* remove OperationSignature from ValidationContext

only used in one rule, does not need to be on context
JoviDeCroock pushed a commit that referenced this pull request Aug 30, 2024
@yaacovCR @JoviDeCroock
validation suggestions (#12)
ae72b44 
* validation suggestions

* remove OperationSignature from ValidationContext

only used in one rule, does not need to be on context

remove dependency on getVariableSignature

move FRAGMENT_ARGUMENT below ARGUMENT

fragment arguments do not have location defaults

only variable defaults => so getDefaultValue(), which returns location defaults for use with the allowedVariableUsage helper, should never be called by getVariableUsages with respect to fragment arguments

fragment arguments therefore need not add anything to the default value stack

reduce diff from main

these diffs crept in before Kind.FRAGMENT_ARGUMENT was separated out
JoviDeCroock pushed a commit that referenced this pull request Aug 30, 2024
@yaacovCR @JoviDeCroock
validation suggestions (#12)
b378b07 
* validation suggestions

* remove OperationSignature from ValidationContext

only used in one rule, does not need to be on context

remove dependency on getVariableSignature

move FRAGMENT_ARGUMENT below ARGUMENT

fragment arguments do not have location defaults

only variable defaults => so getDefaultValue(), which returns location defaults for use with the allowedVariableUsage helper, should never be called by getVariableUsages with respect to fragment arguments

fragment arguments therefore need not add anything to the default value stack

reduce diff from main

these diffs crept in before Kind.FRAGMENT_ARGUMENT was separated out
JoviDeCroock pushed a commit that referenced this pull request Aug 30, 2024
@yaacovCR @JoviDeCroock
validation suggestions (#12)
59beb56 
* validation suggestions

* remove OperationSignature from ValidationContext

only used in one rule, does not need to be on context

remove dependency on getVariableSignature

move FRAGMENT_ARGUMENT below ARGUMENT

fragment arguments do not have location defaults

only variable defaults => so getDefaultValue(), which returns location defaults for use with the allowedVariableUsage helper, should never be called by getVariableUsages with respect to fragment arguments

fragment arguments therefore need not add anything to the default value stack

reduce diff from main

these diffs crept in before Kind.FRAGMENT_ARGUMENT was separated out
JoviDeCroock added a commit that referenced this pull request Aug 30, 2024
@JoviDeCroock @yaacovCR
Fragment arguments validation (#5)
32107eb 
* Fragment args validation

* add experimental substitution

* validation suggestions (#12)

* validation suggestions

* remove OperationSignature from ValidationContext

only used in one rule, does not need to be on context

remove dependency on getVariableSignature

move FRAGMENT_ARGUMENT below ARGUMENT

fragment arguments do not have location defaults

only variable defaults => so getDefaultValue(), which returns location defaults for use with the allowedVariableUsage helper, should never be called by getVariableUsages with respect to fragment arguments

fragment arguments therefore need not add anything to the default value stack

reduce diff from main

these diffs crept in before Kind.FRAGMENT_ARGUMENT was separated out

---------

Co-authored-by: Yaacov Rydzinski <yaacovCR@gmail.com>
@yaacovCR yaacovCR deleted the validation-suggestions branch September 3, 2024 04:44
JoviDeCroock added a commit that referenced this pull request Sep 4, 2024
@JoviDeCroock @mjmahone @yaacovCR
TypeInfo & Validation for fragment-arguments (#4)
a5627c0 
* Add typeinfo functionality as a run-up to supporting the new validation
rules

Co-authored-by: mjmahone <mahoney.mattj@gmail.com>

* Fragment arguments validation (#5)

* Fragment args validation

* add experimental substitution

* validation suggestions (#12)

* validation suggestions

* remove OperationSignature from ValidationContext

only used in one rule, does not need to be on context

remove dependency on getVariableSignature

move FRAGMENT_ARGUMENT below ARGUMENT

fragment arguments do not have location defaults

only variable defaults => so getDefaultValue(), which returns location defaults for use with the allowedVariableUsage helper, should never be called by getVariableUsages with respect to fragment arguments

fragment arguments therefore need not add anything to the default value stack

reduce diff from main

these diffs crept in before Kind.FRAGMENT_ARGUMENT was separated out

---------

Co-authored-by: Yaacov Rydzinski <yaacovCR@gmail.com>

* OverlappingFieldsCanBeMergedRule suggestions (#15)

---------

Co-authored-by: mjmahone <mahoney.mattj@gmail.com>
Co-authored-by: Yaacov Rydzinski <yaacovCR@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoviDeCroock JoviDeCroock JoviDeCroock approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yaacovCR @JoviDeCroock