Skip to content

Add support for autofixing #261

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

benjaminjkraft
Copy link

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
Copy link
Owner

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:

  • 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.)

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

  • It's too bad we don't support --fix in combination with --stdin. I wonder if there's a way around this. 🤔 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. 😢

@benjaminjkraft
Copy link
Author

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. 🤔 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. 😢

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
Copy link
Author

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.

The core 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 autofixers for all of the capitalization and alphabetization
rules.  (The remainder can't (or shouldn't) really be autofixed.)  In
general, capitalization was very easy, and alphabetization was quite
tricky, but in the latter case it's mostly in a shared util that handles
all of annoying syntax-transformation junk.

Fixes cjoudrey#23.
@benjaminjkraft
Copy link
Author

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

@eMerzh
Copy link

eMerzh commented Dec 24, 2020

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

@eMerzh
Copy link

eMerzh commented Dec 24, 2020

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
Copy link
Author

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.

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.

3 participants