Since this isn't directly related to the PR in question, I'm starting a
new thread.
On 12/20/2019 7:22 PM, Philip Race wrote:
On 12/20/19, 7:04 PM, Scott Palmer wrote:
I'm not sure if I'me supposed to try to integrate now that I've made
that 10 -> 0 change, or if the new change resets the need for review...
It shows ready, which surprises me.
Still learning skara .. I'd expect any change to reset as how can it
know if it is
a good or insignificant change or not no matter how the commenter
characterised the issue ?
Pushing a new commit doesn't automatically invalidate existing
approvals, although it is highlighted in the "Approvers" section that
the approval was for a prior commit:
Approvers
Phil Race (prr - Reviewer) Note! Review applies to f846ad6
I remember bringing this up with the Skara team during my initial
testing, since I also had wondered whether pushing a new commit should
invalidate approvals. The current policy allows for doing trivial fixes
brought up when approving a review (e.g., a change in a comment or
formatting) without requiring all reviewers to re-approve. It matches
current practice for email-based reviews, so I think the current
behavior is a reasonable default.
They did say that they could implement an optional feature (i.e., a
project would need to opt-in to this behavior, probably via a
.jcheck/conf property) to invalidate all approvals upon pushing a new
commit, but I don't think an RFE was filed to track it.
This seems like it could be a valuable feature, although it does come
with a slight cost in terms of timing and flexibility. What do others think?
-- Kevin
