Should Skara tooling invalidate approvals when new commits are pushed? [was: RFR: 8130738: Add tabSize property to Text and TextFlow]

[Kevin Rushforth]

Does anyone strongly feel otherwise? If not, then I'll request the Skara 
team to enable this feature.

-- Kevin


On 1/8/2020 11:26 AM, Johan Vos wrote:
> I agree. If the fix is trivial, the time for the reviewer will be 
> really short. The more complex the fix, the more time it will take -- 
> with the risk of being delayed to the next release.
> The github interface makes it very easy to inspect the new commit, 
> hence a typo in javadoc can easily be fixed and re-approved.
>
> I think this approach is safer than the approach were "trivial" fixes 
> don't need re-approval, as that approach leaves more room for 
> interpretation (and probably even more interactions with the reviewers 
> ("is this a trivial fix?")).
>
> - Johan
>
> Op wo 8 jan. 2020 om 19:34 schreef Nir Lisker <nlisker at gmail.com 
> <mailto:nlisker at gmail.com>>:
>
>     Looks like an all-or-nothing situation - either any commit requires
>     re-approval or no commit requires re-approval. In this case, I
>     would say
>     that all commits should require re-approval since it's the safer
>     approach.
>     Having the issue stay in approved state after a significant change
>     is much
>     worse than requiring the reviewers to take a look at an insignificant
>     commit and re-approve. The time is takes for a reviewer to go over a
>     comment or formatting change is short and, I believe, not intrusive to
>     their workflow.
>
>     On Tue, Dec 31, 2019 at 7:48 PM Kevin Rushforth
>     <kevin.rushforth at oracle.com <mailto:kevin.rushforth at oracle.com>>
>     wrote:
>
>     > 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
>     >
>     >
>