search

cjoudrey / graphql-schema-linter

Validate GraphQL schema definitions against a set of rules
MIT License
694 stars 62 forks source link

Possible additional rules #216

Closed aldeed closed 4 years ago

aldeed commented 4 years ago

There are some rules we've created that I think would be generally useful to others. Would you accept a PR that adds these?

Definitions: https://github.com/reactioncommerce/reaction/tree/trunk/.reaction/graphql-linter/rules

cjoudrey commented 4 years ago

Hey Eric,

Thanks for opening the issue. 😄 I'm super happy that you wrote custom rules and are now considering upstreaming them. 🎉

arguments-have-descriptions: Every argument must have a description

:+1: I'm surprised this one didn't already exist. I must have forgotten.

descriptions-are-capitalized: All description text must start with a capital letter

:+1:, but with a small subtly. Descriptions are defined using Markdown syntax as per the GraphQL spec. Although most people probably just use plain text, I wouldn't want the linter to start complaining because someone's description begins with * or _ (for example). I noticed your implementation checks description[0] == description[0].toUpperCase() which would technically still work since "*" == "*".toUpperCase().

Let me know what you think about this one.

input-object-fields-sorted-alphabetically: Fields of an input must be listed alphabetically type-fields-sorted-alphabetically: Fields of a type must be listed alphabetically

Out of curiosity, what's your workflow like to sort these alphabetically? Do you write the .graphql schema files by hand or are they auto-generated by a tool you use? I'm asking because I'd really hate to have to manually sort fields by hand or find the right spot to insert a new field on an existing type.

aldeed commented 4 years ago

@cjoudrey We write by hand and people manually add fields in the correct place. It was annoying to fix when we first added the rule but adding new ones in the correct place hasn't really been a problem for people. It makes it a lot easier to skim and see whether a field exists, plus not all client schema docs alphabetize them automatically so it makes the docs more readable, too. (like GraphiQL, Playground, etc.)

That said, alphabetizing rules would be pretty easy to --fix if that were implemented someday.

Good point about markdown support in descriptions. We do have many descriptions with markdown in them and I haven't seen any issues with this, but most likely few of them start with markdown characters. Mostly we only use it to bold, italic, or code format. I think even things like a dash or a square bracket (for a link) would pass the test. It's probably more likely to miss issues than to complain unnecessarily.

descriptions-are-capitalized is not a super important rule, but we added it because we generate static docs from the schemas and the docs are more readable and correct that way. Others may have the same needs. Do you have any ideas for a way to support markdown properly? I think unless we actually run them through a markdown conversion in the rule function, it can't be perfect. That would be a big extra dependency, so in that case it should probably be a separate package.

cjoudrey commented 4 years ago

Do you have any ideas for a way to support markdown properly?

I think what you did is good enough. 😄I don't think it's worth making it properly support markdown for now.

alphabetizing rules would be pretty easy to --fix if that were implemented someday.

Absolutely agree! Let's add it.

aldeed commented 4 years ago

@cjoudrey PR is here: https://github.com/cjoudrey/graphql-schema-linter/pull/219