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 <[email protected]
<mailto:[email protected]>>:
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
<[email protected] <mailto:[email protected]>>
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
>
>
