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.

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

Eslint config validation #86

Merged
merged 8 commits into from
Sep 7, 2017
Merged

Eslint config validation #86

merged 8 commits into from
Sep 7, 2017

Conversation

lydell
Copy link
Contributor

@lydell lydell commented Aug 26, 2017 •
edited
Loading

TODO:

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests pass
  • Update CHANGELOG.md with your change
  • If this was a change that affects the external API, update the README

Fixes #85

lydell added 6 commits August 26, 2017 15:30
@lydell
Fix .graphqlconfig support for "graphql/capitalized-type-name"
d74ca85 
When trying to use the "graphql/capitalized-type-name" rule with a
.graphqlconfig instead of the `schema*` options, this error was
previously reported by ESLint:

    Configuration for rule "graphql/capitalized-type-name" is invalid:
      Value "[object Object]" should have required property '.schemaJson'.
      Value "[object Object]" should have required property '.schemaJsonFilepath'.
      Value "[object Object]" should have required property '.schemaString'.
      Value "[object Object]" should match exactly one schema in oneOf.

With this commit it works as expected.

This was done by syncing the ESLint config schema for the
"graphql/capitalized-type-name" rule with the other rules.
@lydell
Allow specifying the env option in all rules
2ffb096 
Previously, the ESLint config schema only allowed the `env` option for
the "graphql/template-strings" rule and the "graphql/required-fields"
rule.

This commit allows the `env` option for the "graphql/named-operations"
rule and the "graphql/capitalized-type-name" rule as well, by moving the
ESLint config schema for the `env` option to the schema part shared by
all rules.

The `env` option is valuble to all rules, since it is used for the
slight parsing differences in the GraphQL syntax between apollo, relay
and lokka.
@lydell
Fix missing rule options validation
3c9ea1b 
Previously, providing no options at all to any of the rules provided by
this plugin resulted in nothing happening at all. Examples:

    "graphql/template-strings": "error",
    "graphql/required-fields": ["error"],

That's not very helpful, and clearly not intended since `minLength: 1`
was specified in the ESLint config schema for all rules. However, the
correct name is "minItems", not "minLength". (I suspect this error comes
from confusion between different JSON schema versions.)

This commit changes "minLength" to "minItems", resulting in this error
being reported by ESLint (for example):

    Configuration for rule "graphql/template-strings" is invalid:
    Value "" should NOT have less than 1 items.
@lydell
Fix crash in "graphql/required-fields"
f7f305e 
Previously, this configuration:

    "graphql/required-fields": ["error", {}],

... resulted into this crash:

    TypeError: Cannot read property 'forEach' of undefined
        at Object.Field (/eslint-plugin-graphql/lib/rules.js:31:21)

That's because the `requiredFields` option of the
"graphql/required-fields" rule is required, but wasn't marked as such in
the ESLint config schema.

This commit marks the `requiredFields` option as required, resulting in
this error being reported by ESLint:

    Configuration for rule "graphql/required-fields" is invalid:
    Value "[object Object]" should have required property 'requiredFields'.

It would be nice to also add `minItems: 1` so that the following
configuration would be invalid as well:

    "graphql/required-fields": ["error", {requiredFields: []}],

But that would be a breaking change, so that wasn't included.
@lydell
Clarify the "graphql/required-fields" rule examples
19d7b4d 
The three examples are so similar that it is hard to see the difference
without a little bit of text guiding you.
@lydell
Improve docs regarding common options
c59f35b
@apollo-cla
Copy link

@lydell: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@lydell lydell mentioned this pull request Aug 26, 2017
Closed
@lydell
Copy link
Contributor Author

lydell commented Aug 29, 2017

@stubailo @jnwng This is supposed to be an easy-to-merge non-controversial PR that fixes a couple of smaller bugs. Most importantly, it makes .graphqlconfig work with the graphql/capitalized-type-name rule. Is there anything you'd like me to change before merging? :)

@jnwng
Copy link
Contributor

jnwng commented Aug 29, 2017 via email

jnwng
jnwng approved these changes Aug 29, 2017
View reviewed changes
Copy link
Contributor

@jnwng jnwng left a comment

Choose a reason for hiding this comment

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

one nit if you dont mind addressing / responding @lydell but otherwise lgtm

README.md Outdated
@@ -246,6 +270,24 @@ module.exports = {
}
```

When using `.graphqlconfig`, you might end up not needing to specify any additional options for a rule. Note that you still _have_ to provide an empty object (`{}`) to the rule:
Copy link
Contributor

Choose a reason for hiding this comment

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

this API is a little strange to suggest. can we default the optionGroup to an empty object? that should (hopefully) resolve some of this wonkiness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see if I can come up with something :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed a fix now. (Note that this would also be fixed by #87).

@lydell
Allow specifying no options at all for some rules
cd5aed7 
For rules which have no required options for every option group it is
valid to specify no configuration at all:

    "graphql/named-operations": "error",
    "graphql/capitalized-type-name": "error",
    "graphql/template-strings": "error",

The above is now equivalent to:

    "graphql/named-operations": ["error", {}],
    "graphql/capitalized-type-name": ["error", {}],
    "graphql/template-strings": ["error", {}],

Note that this is still a config error, because the `requiredFields`
option is required:

    "graphql/required-fields": "error",
@lydell
Copy link
Contributor Author

lydell commented Sep 4, 2017

@jnwng Is there anything else you’d like me to do in this PR? :)

@jnwng jnwng merged commit e48ae8f into apollographql:master Sep 7, 2017
@jnwng
Copy link
Contributor

jnwng commented Sep 7, 2017

nope, just maybe find me more time in the day :)

thank you @lydell!

@lydell lydell deleted the eslint-config-validation branch September 8, 2017 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnwng jnwng jnwng 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.
3 participants
@lydell @apollo-cla @jnwng