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

[Kevin Rushforth]

Hearing no objections, we will enable this for pull requests on the jfx 
repo starting Tuesday, Feb 4.

-- Kevin


On 1/8/2020 11:34 AM, Kevin Rushforth wrote:
> 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
>>     >
>>     >
>>
>