Skip to content

validation suggestions #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Aug 28, 2024
Merged

validation suggestions #12

merged 2 commits into from
Aug 28, 2024

Conversation

yaacovCR
Copy link
Collaborator

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 GitHub Actions
Copy link

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
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

src/validation/rules/NoUnusedFragmentArgumentsRule.ts
Copy link
Owner

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

src/validation/rules/NoUnusedFragmentVariablesRule.ts
Copy link
Owner

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

src/validation/rules/NoUnusedVariablesRule.ts
Copy link
Owner

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
Collaborator Author

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
14 of 15 checks passed
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 <[email protected]>
@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 <[email protected]>

* 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 <[email protected]>

* OverlappingFieldsCanBeMergedRule suggestions (#15)

---------

Co-authored-by: mjmahone <[email protected]>
Co-authored-by: Yaacov Rydzinski <[email protected]>
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