RFR: 2331: Improve review notes
Pavel Rappo
prappo at openjdk.org
Fri Jul 19 10:26:40 UTC 2024
On Fri, 19 Jul 2024 10:06:05 GMT, Pavel Rappo <prappo at openjdk.org> wrote:
>> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 616:
>>
>>> 614: + "](" + pr.filesUrl(hash.get()) + "))";
>>> 615: } else {
>>> 616: entry += " ⚠️ Review applies to [" + hash.get().abbreviate()
>>
>> Now that 🔄 and ⚠️ can appear on 2 different reviewers in the same PR, we should consider changing this warning symbol to some other emoji that indicates "outdated".
>
> Rest assured, I will not integrate this PR until we figure out what to do with the issue you raised, Chen. That issue should be fixed either in this PR or a follow-up one.
>
> Personally, I think if a review is stale but otherwise good, we don't need to add any emoji. It suffices to add the commit hash the review applies to, and maybe indicate how far away that commit is from HEAD, to quantify staleness: for example, "3 commits behind". Any extra commits added since that review may only be simple merges anyway.
>
> However, if a review is stale and restricted (i.e. does not count in required reviews), we may add "⚠️". In a sense, it would provide a similar indication to that of a repo configured with:
>
> useStaleReviews=true
> acceptSimpleMerges=false
>
> In this picture, the "🔄" emoji goes away completely.
To add to my previous message.
We should keep in mind that any indication in question is primarily for a PR _author_. From my own experience as a _reviewer_, I don't normally follow a particular PR that I have reviewed to see if my review has become stale and needs an update. If the author feels I should re-review their PR, they ping me directly.
-------------
PR Review Comment: https://git.openjdk.org/skara/pull/1679#discussion_r1684176753
More information about the skara-dev
mailing list