search

cjoudrey / graphql-schema-linter

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

Add support for autofixing #261

Open benjaminjkraft opened 4 years ago

benjaminjkraft commented 4 years ago

The API is fairly simple: errors can have a new field, fix, which includes a location (likely a graphql node's .loc) and a replacement. The runner then takes care of applying the fix, if the --fix option was specified.

I added just one autofixer in this commit, for enum-rules-all-caps. It should be fairly easy to add more, I just figured I'd keep this commit small by adding only a very simple one. (Full disclosure: the rules I actually care about autofixing are custom plugins!)

Fixes (some of) #23.

cjoudrey commented 4 years ago

Thanks for your contribution @benjaminjkraft!

I love that you're working on adding support for --fix. It's something I've been wanting to add for a while. 😄

I quickly looked at your pull request and a few things come to mind:

benjaminjkraft commented 4 years ago

While I understand you're most interested in fixing custom rules, I'm a bit hesitant to deciding on an API for fixing errors without ensuring that this API will work for all the built in rules (or, at least the rules that are fixable.)

Totally fair! I'll see what I have time to do later this week.

When writing the schema back, we need to make sure this is compatible with multi-file schemas. You might already be handling this, but it wasn't obvious to me when I looked at the tests.

I am certainly trying to handle that right (using the SourceMap), but I realize you're right that it isn't really tested. I'll see if I can figure out the plumbing needed to do so.

It's too bad we don't support --fix in combination with --stdin. I wonder if there's a way around this. :thinking: I was thinking perhaps we could print the fixed schema to stdout and the errors to stderr, but then I realized we currently print errors to stdout. :cry:

Yeah, I agree it's unfortunate -- if we can come up with a good interface it should be easy to implement. (And will save me some time re-wiring a script that currently uses --stdin!) One option would be to take your idea, but put it behind a new flag --stderr which says to print errors to stderr (and could be used with or without --stdin, --fix, etc.). Then the rule would be that you can use --stdin and --fix only if you also use --stderr. A somewhat confusing API surface, for sure, but at least it makes everything one might want possible. (Presumably most people using --stdin are using it from scripts or tools anyway, you would know better than I though.)

benjaminjkraft commented 4 years ago

Ok, I've added support for all the capitalization-related fixers, and wrote some more tests for the fix-application logic (which indeed caught a bug). Autofixing the alphabetization rules is surprisingly tricky (due to keeping track of the whitespace and comments between blocks); I may have more time tomorrow to try to finish that off.

benjaminjkraft commented 4 years ago

Ok, unless I missed any, all the rules that are fixable should have fixers now!

eMerzh commented 3 years ago

thanks @benjaminjkraft it worked like a charm for my schema :) well done... hope this can get rebased / merged soon ... :)

eMerzh commented 3 years ago

note, on another test i got a small mixup with the rule of the case... got

  views: Int
  unique_views: Int

which ends up as

unique_views: Int
views: IntuniqueViews: Int
benjaminjkraft commented 3 years ago

Oh, good find! It looks like I refactored out the check I had for conflicts between fixes. I suspect we won't be able to fix that one in one go (without a lot of work, anyway) but it shouldn't be too hard to add a conflict check and just apply the first fix (then running twice will fix it fully). I'll take a look at that, although I may not have time for a week or two.