yari icon indicating copy to clipboard operation
yari copied to clipboard

Move flaw details in PR reviewer to GitHub Annotations

Open nschonni opened this issue 3 years ago • 5 comments

I think the preview URLs are best suited as a comment on the PR, but it might be better to use the Checks/Annotations/Problem Matcher setup for the flaws portion of the PR check. I know getting back to the specific line numbers for some of the flaw results might not be possible, but I think setting the "line" part of the match to 1, would just add the comment on the first line of the file.

Some related discussions on https://github.com/mdn/yari/pull/2753

nschonni avatar Apr 10 '21 20:04 nschonni

@ddbeck Was this your suggestion too?

peterbe avatar Apr 12 '21 18:04 peterbe

In principle, I like that idea. We use PyGitHub which is a very capable lib for posting comments. So it probably would be quite possible. But it's a bit less trivial. Worth a shot maybe.

Also, if a flaw does not have a line we could simply say that in the comment which might explain the confusion when someone looks at line 1 and wonders "What's that got to do with this line??"

peterbe avatar Apr 12 '21 19:04 peterbe

In principle, I like that idea. We use PyGitHub which is a very capable lib for posting comments. So it probably would be quite possible. But it's a bit less trivial. Worth a shot maybe.

If you add and register a problem matcher https://github.com/mdn/content/pull/1206/files#diff-25c661e20a38ae8e4610534138256b0595a4703ee1a06430bc22add458e56b8dR1 you don't even need the PyGitHub part at all 😄 . It creates those annotations based on the format of the messages the deployment would print to the log stream

nschonni avatar Apr 13 '21 01:04 nschonni

Oh I see. I guess I could have guessed it was a specific thing when you wrote "GitHub Annotations" as a noun. So it's not simply PR comments for specific lines on specific lines.

If you want to experiment more deeply with this, we can use my fork of mdn/content and we can set up some PRs within that fork which we can bombard to test things. I'd love to learn more about how this works because right now I know next to nothing.

peterbe avatar Apr 13 '21 14:04 peterbe

@ddbeck Was this your suggestion too?

Yes, pretty much. I was thinking of setting the checks run output.summary and output.text (the properties of the output object in the GitHub docs) to the Markdown that currently goes into the bot comment, but using annotations would be a nice upgrade (especially for stuff that's per-file). I think @nschonni has it right that a problem matcher would be easier to implement, too.

ddbeck avatar Apr 13 '21 14:04 ddbeck