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

[review bot] Add comment to the PR instead of printing something to console that nobody reads #34549

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

autoantwort
Copy link
Contributor

Only one comment is added to a PR and then this comment is updated

@autoantwort autoantwort force-pushed the raise-the-bot-again-from-the-ashes branch 15 times, most recently from 75e651c to c75d2b2 Compare October 17, 2023 23:07
@autoantwort autoantwort force-pushed the raise-the-bot-again-from-the-ashes branch from c75d2b2 to a188007 Compare October 17, 2023 23:10
@autoantwort
Copy link
Contributor Author

I have tested also at my fork :)

@LilyWangLL LilyWangLL added category:infrastructure Pertaining to the CI/Testing infrastrucutre info:reviewed Pull Request changes follow basic guidelines labels Oct 18, 2023
@LilyWangLL LilyWangLL requested review from BillyONeal and removed request for BillyONeal October 18, 2023 06:17
@JavierMatosD JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Oct 18, 2023
@BillyONeal
Copy link
Member

  1. I think it would be better to fix the code review comment that is left to do the explaining rather than trying to smash things into a comment.
  2. We used to do a comment, and backed that off due to confusion over which specific changes were being commented on, as we were essentially just duplicating the checks UI on top of a system not designed to be the checks UI. Has anything changed about that situation vs. Get the Actions bot out of the PR review business. #29777 when the comment was removed?

@JavierMatosD JavierMatosD removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Oct 18, 2023
@JavierMatosD JavierMatosD marked this pull request as draft October 18, 2023 17:51
@BillyONeal
Copy link
Member

Is this related to microsoft/vcpkg-tool#720 ? (As in, if these checks are in the tool can we emit better file/line info which makes the check work better?)

@autoantwort
Copy link
Contributor Author

I think it would be better to fix the code review comment that is left to do the explaining rather than trying to smash things into a comment.

So do everything as code annotations like the deprecated warnings?

We used to do a comment, and backed that off due to confusion over which specific changes were being commented on, as we were essentially just duplicating the checks UI on top of a system not designed to be the checks UI. Has anything changed about that situation vs. #29777 when the comment was removed?

In the past for every push a new comment/review was generated so that there were multiple review comments after multiple pushes. Now there is only one comment for the latest changes. If the points gets fixed the comment gets removed.

Is this related to microsoft/vcpkg-tool#720 ? (As in, if these checks are in the tool can we emit better file/line info which makes the check work better?)

Hm no not really. We could also emit better file/line info in this PR.

@BillyONeal
Copy link
Member

In the past for every push a new comment/review was generated so that there were multiple review comments after multiple pushes. Now there is only one comment for the latest changes. If the points gets fixed the comment gets removed.

Issues with this:

(1) if the bot gets unhappy for some reason, the old comment with inaccurate information is left behind, and sometimes it can be unclear if the content there is current
(2) github aggressively tries to hide old comments

My overall opinion is that what we are doing is a check, so we should exhaust every avenue through the checks system to do what we want, rather than trying to make our own checks UI. If we are going to make our own checks UI that operates through comments, I think we need a concrete list of 'these are the problems we are trying to solve with that and this is why the checks UI can't be fixed to do that'.

If people are confused by our error messages and therefore don't know what to do, moving that error message into a comment doesn't fix the problem. If we've tried everything through the checks and annotations mechanism and consistently as soon as people see the message they know what to do (meaning it is only a visibility problem), then bringing in this sort of comment system might make sense.

It looks like the status quo is that people don't read the warnings or errors because they are (1) confusing, or (2) in a sea of console spew, and all we need to do to get the checks UI to do the right thing is prefix the resulting lines with the right prefixes.

@data-queue data-queue removed the info:reviewed Pull Request changes follow basic guidelines label Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:infrastructure Pertaining to the CI/Testing infrastrucutre
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants