Should Skara tooling invalidate approvals when new commits are pushed? [was: RFR: 8130738: Add tabSize property to Text and TextFlow]
kevin.rushforth at oracle.com
Wed Jan 8 19:34:22 UTC 2020
Does anyone strongly feel otherwise? If not, then I'll request the Skara
team to enable this feature.
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
> 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>>
> > 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
> > 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
> > 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
> > 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
> > with a slight cost in terms of timing and flexibility. What do
> > think?
> > -- Kevin
More information about the openjfx-dev