Skip to content
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.

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

extend require-description lint with operation field definitions #1303

Merged
merged 7 commits into from
Dec 20, 2022

Conversation

tshedor
Copy link
Contributor

@tshedor tshedor commented Dec 14, 2022

Description

This change extends the require-description lint to require a description of operations.

# Correct
type Query {
  "Fetch users"
  users: [User!]!
}

# Incorrect
type Mutation {
  createUser(user: UserInput!): User!
}

The field definition within the operation types is tricky to target. A description isn't required for every Kind.FIELD_DEFINITION so the existing feature would've involved excessive documentation. Since good code health could require documentation of usable operations, the lint was extended to target field definitions only within the scope of Mutation, Query, and Subscription.

This adds the boolean option operationFieldDefinition to require-description (default false).

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

I added two valid unit tests and two invalid unit tests. They cover when an input is present and when an input is not present.

Both unit tests are within the require-description.spec.ts file

Test Environment:

  • OS: Mac OS Monterrey
  • @graphql-eslint/...: master?
  • NodeJS: 18.3.0

Checklist:

  • I have followed the
    CONTRIBUTING doc and the
    style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings (They add a new ESLint warning that's configurable and default disabled)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests and linter rules pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules (N/A)

@changeset-bot
Copy link

changeset-bot bot commented Dec 14, 2022

🦋 Changeset detected

Latest commit: 379ace3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@graphql-eslint/eslint-plugin Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Josiassejod1
Copy link

Thanks for doing this

@dimaMachina
Copy link
Owner

Thanks for working on it! I definitely see the benefit of the new option, and will review it soon!

@@ -147,6 +162,10 @@ const rule: GraphQLESLintRule<[RequireDescriptionRuleConfig]> = {
}
}

if (operationFieldDefinition) {
kinds.add(':matches(ObjectTypeDefinition, ObjectTypeExtension)[name.value=/Query|Mutation|Subscription/] > FieldDefinition');
Copy link
Owner

Choose a reason for hiding this comment

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

this will not work for root types that are named differently.
example:

type MyQuery {
  user: User
}

schema {
  query: MyQuery
}

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@tshedor tshedor Dec 15, 2022

Choose a reason for hiding this comment

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

These have been added, but they're hardcoded to avoid requiring the schema for the whole rule. If you'd rather these not be hardcoded and to match no-root-type's dynamic fetching of the names, I'd be happy to convert this rule so that the schema is exposed to it.

Comment on lines 226 to 230
{
code: 'type Query { users: [User!]! }',
options: [{ operationFieldDefinition: true }],
errors: [{ message: 'Description is required for `Query.users`.' }],
},
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
{
code: 'type Query { users: [User!]! }',
options: [{ operationFieldDefinition: true }],
errors: [{ message: 'Description is required for `Query.users`.' }],
},

one test for Query is enough, instead include 1 test with a root type that is named differently than Query/Mutation or Subscription

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@B2o5T I added a test case for a root type with a field as a Correct case. Is this what you had in mind? If I misread you I'd be happy to try again

…ns within `Query`, `Mutation`, and `Subscription` root types
Copy link
Owner

@dimaMachina dimaMachina left a comment

Choose a reason for hiding this comment

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

thanks, will be a part of next release

@dimaMachina dimaMachina merged commit c6d5bb7 into dimaMachina:master Dec 20, 2022
dimaMachina added a commit that referenced this pull request Dec 20, 2022
* require description for operation field definitions

* Update packages/plugin/tests/require-description.spec.ts

Co-authored-by: Victor Rojas <Victorrent@users.noreply.github.com>

* narrow query

* pr feedback

* format

* [require-description] add `rootField` option for only field definitions within `Query`, `Mutation`, and `Subscription` root types

Co-authored-by: Victor Rojas <Victorrent@users.noreply.github.com>
Co-authored-by: Dimitri POSTOLOV <dmytropostolov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants