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

Turn fixits into suggestions #4

Closed
ZedThree opened this issue Dec 1, 2020 · 5 comments · Fixed by #16
Closed

Turn fixits into suggestions #4

ZedThree opened this issue Dec 1, 2020 · 5 comments · Fixed by #16

Comments

@ZedThree
Copy link
Owner

ZedThree commented Dec 1, 2020

Rather than trying to work out if a comment is really a fixit, maybe try getting clang-tidy to fix everything, and then take the diff of that and add as a suggestion?

@vadi2
Copy link
Contributor

vadi2 commented Feb 22, 2021

That would be great 👍

@vadi2
Copy link
Contributor

vadi2 commented Apr 20, 2021

Just checking, any bandwidth to add this in?

@ZedThree
Copy link
Owner Author

Probably not for a few weeks I'm afraid

@ZedThree
Copy link
Owner Author

I think I've found an easy-ish way to do this with the --export-fixes option, which exports to YAML.

@ZedThree
Copy link
Owner Author

So, annoyingly, the export-fixes yaml file just gives character offsets into the file, and not line numbers. This makes it very easy to apply the fix, but less easy to turn it into a Github suggestion fixit.

One way to use the existing method would be to look for multi-line warning bodies where the second line is a '^' by itself. Then the third line is what needs to be inserted. This is quite easy to do.

But there's still a couple of issues to do with fixes spanning multiple lines. For example, a fix might also require including a header. It might not be possible to put a suggestion comment to fix that if the line is not in the PR diff. There's probably more or less elegant ways of handling that.

The other one being something like wrapping a statement in braces. The output of clang-tidy (which is what we currently parse), only shows the first brace, whereas the exported fixes file has both.

Sooo, a complete but complicated method might be:

  1. run clang-tidy with --export-fixes=<file>.yaml
  2. read this file into a dict but convert this to be {"file": {<line>: ...}} -- or better, make a lookup map like we do for the source file <--> diff line numbers
  3. in make_review, when we find a warning, look up the file/line in the structure from step 2
  4. check each replacement:
    1. if its offset corresponds to the warning line, if so make it a suggestion;
    2. if not, check if it's in the diff, then make it's own comment and suggestion;
    3. otherwise, add it as a normal code block to this comment with a note that it needs to be applied manually

Now I've thought this through and worked out how to construct the lookup map in a non-terrible way, I will have a stab at doing it this weekend.

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 a pull request may close this issue.

2 participants