search

flowwer-dev / pull-request-stats

Github action to print relevant stats about Pull Request reviewers
MIT License
366 stars 79 forks source link

Remove -review:none from pull request query #70

Closed damccorm closed 1 year ago

damccorm commented 1 year ago

Right now, this action filters pull requests using -review:none which is supposed to filter out pull requests that haven't been reviewed. Unfortunately, this includes pull requests which have been reviewed with comments, but not approvals or "request changes", so it causes reviews to be systematically undercounted.

For example, you can see this query returns results even though both reviewed-by:damccorm and -review:none are specified - https://github.com/apache/beam/pulls?q=is%3Apr+is%3Aopen+reviewed-by%3Adamccorm+review%3Anone+-author%3Adamccorm

I ran the forked version of this action and it fixed some accuracy issues I'd noticed.

Before the change (using flowwer-dev/pull-request-stats@master): https://github.com/damccorm/playground/pull/27 After my change (using damccorm/pull-request-stats@master): https://github.com/damccorm/playground/pull/29

Note the higher counts, despite them being run on the same morning (and I didn't do any pr reviews between runs).

damccorm commented 1 year ago

Not sure why the diff is so large on the generated dist file, but if you do a diff on the contents between master and my changes you shouldn't notice anything other than the -review:none change

manuelmhtr commented 1 year ago

Hi @damccorm, thanks for such a complete report on the issue. I'm currently on vacation, but please give me one week to return, and deeply analyze the implications of the change.

Best!

damccorm commented 1 year ago

Sounds good, thank you! Enjoy the vacation

manuelmhtr commented 1 year ago

Hi @damccorm Thanks for your contribution. Removing the filter and including PRs with requested changes to the stats made a lot of sense. It's now published on v2.9.0.

By the way, I saw you are working on returning the results as an output or a file. It seems very interesting. What are you trying to achieve?

damccorm commented 1 year ago

Thank you! (and sorry for the slow response, it was my turn to be on vacation 😃)

By the way, I saw you are working on returning the results as an output or a file. It seems very interesting. What are you trying to achieve?

I was trying to think about a generic way to make the results of the action available to a future step in a user's workflow. For my specific use case, I'm interested in sending an email with the results to a distribution list (the apache/beam developer email list) since we mostly communicate there.

I could add another connector (like the webhook or slack url), but email requires a little more config (sender email address and password, recipient email address, email service) which might crowd the options, so I was trying to come up with a more composable/generalizable solution. I'm not sure if this is the right path yet though, other alternatives I can think of include trying to grab the step summary that we're already posting for the actions run or setting outputs with the results directly instead of writing them to a file.

damccorm commented 1 year ago

I filed #72 since I won't have time to get to this myself in the near term