You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@baremaps.apache.org by Bertil Chapuis <bc...@gmail.com> on 2022/11/14 20:27:52 UTC
Reviews of pull requests
Hi Everyone,
The current policy is to have at least one review before merging. Some changes need to be reviewed. However, I believe that many incremental changes (config, cleaning, typos, etc.) can be reviewed after being merged. I'd like committers to feel empowered to modify and improve the code base when needed. Do you think we should relax the current policy and disable the review requirement?
Best,
Bertil
Re: Reviews of pull requests
Posted by Josh Fischer <jo...@joshfischer.io>.
Bertrand,
Thank you for the article. I’ve never heard this term before.
My favorite bullet point from the first article is:
- Radiating intent shows others that adventurous behavior is acceptable
in the org.
I learned something new today. +1 to you, sir.
On Wed, Nov 16, 2022 at 4:33 AM Bertrand Delacretaz <bd...@apache.org>
wrote:
> Hi,
>
> Josh Fischer <jo...@joshfischer.io> wrote:
> > ...I do find it helpful if I leave a PR open for 12-24
> > hours to people a chance to make any comments before we merge...
>
> +1, that's a great way of "radiating intent" [1], leaving a chance for
> others to object without slowing down too much.
>
> -Bertrand
>
> [1]
> https://medium.com/@ElizAyer/dont-ask-forgiveness-radiate-intent-d36fd22393a3
> (and also https://davidmarquet.com/turn-the-ship-around-book/ )
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@baremaps.apache.org
> For additional commands, e-mail: dev-help@baremaps.apache.org
>
> --
Sent from A Mobile Device
Re: Reviews of pull requests
Posted by Andrea Borghi <an...@gmail.com>.
I agree with the fact that ctr can be a very effective way of pushing
forward. Ok for me to allow it, it is then to the pr author to consider
when he feels that a review is required before merging.
I'm really not used to this way, it will be interesting to see how this
performs ;)
Andrea
On Wed, Nov 16, 2022, 13:31 Bertil Chapuis <bc...@gmail.com> wrote:
> Thanks a lot for all your feedback.
>
> Josh, your idea is balanced. Opening the PR and tagging reviewers leaves
> the opportunity to comment or to object to a change.
>
> The branch protection rules have been disabled.
>
> Bertil
>
> > On 16 Nov 2022, at 09:33, Bertrand Delacretaz <bd...@apache.org>
> wrote:
> >
> > Hi,
> >
> > Josh Fischer <jo...@joshfischer.io> wrote:
> >> ...I do find it helpful if I leave a PR open for 12-24
> >> hours to people a chance to make any comments before we merge...
> >
> > +1, that's a great way of "radiating intent" [1], leaving a chance for
> > others to object without slowing down too much.
> >
> > -Bertrand
> >
> > [1]
> https://medium.com/@ElizAyer/dont-ask-forgiveness-radiate-intent-d36fd22393a3
> > (and also https://davidmarquet.com/turn-the-ship-around-book/ )
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@baremaps.apache.org
> > For additional commands, e-mail: dev-help@baremaps.apache.org
> >
>
>
Re: Reviews of pull requests
Posted by Bertil Chapuis <bc...@gmail.com>.
Thanks a lot for all your feedback.
Josh, your idea is balanced. Opening the PR and tagging reviewers leaves the opportunity to comment or to object to a change.
The branch protection rules have been disabled.
Bertil
> On 16 Nov 2022, at 09:33, Bertrand Delacretaz <bd...@apache.org> wrote:
>
> Hi,
>
> Josh Fischer <jo...@joshfischer.io> wrote:
>> ...I do find it helpful if I leave a PR open for 12-24
>> hours to people a chance to make any comments before we merge...
>
> +1, that's a great way of "radiating intent" [1], leaving a chance for
> others to object without slowing down too much.
>
> -Bertrand
>
> [1] https://medium.com/@ElizAyer/dont-ask-forgiveness-radiate-intent-d36fd22393a3
> (and also https://davidmarquet.com/turn-the-ship-around-book/ )
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@baremaps.apache.org
> For additional commands, e-mail: dev-help@baremaps.apache.org
>
Re: Reviews of pull requests
Posted by Bertrand Delacretaz <bd...@apache.org>.
Hi,
Josh Fischer <jo...@joshfischer.io> wrote:
> ...I do find it helpful if I leave a PR open for 12-24
> hours to people a chance to make any comments before we merge...
+1, that's a great way of "radiating intent" [1], leaving a chance for
others to object without slowing down too much.
-Bertrand
[1] https://medium.com/@ElizAyer/dont-ask-forgiveness-radiate-intent-d36fd22393a3
(and also https://davidmarquet.com/turn-the-ship-around-book/ )
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@baremaps.apache.org
For additional commands, e-mail: dev-help@baremaps.apache.org
Re: Reviews of pull requests
Posted by Josh Fischer <jo...@joshfischer.io>.
I’m fine either way. I do find it helpful if I leave a PR open for 12-24
hours to people a chance to make any comments before we merge. But then
again waiting can be a bit progress killer at times.
On Tue, Nov 15, 2022 at 2:16 PM Julian Hyde <jh...@apache.org> wrote:
> The "CTR vs RTC" discussion is an important one for a community to
> have. In my opinion, there's no easy answer. (I'm basically agreeing
> with Bertrand here.) It is certainly useful to have a default process,
> and that default process should probably be RTC. But also develop
> trust so that people who have gained merit can move faster. ("Just do
> it" and "Small reversible steps" are ASF mantras that support that
> philosophy.)
>
> On Tue, Nov 15, 2022 at 4:42 AM Bertil Chapuis <bc...@gmail.com> wrote:
> >
> > Thank you Andrea and Bertrand for your answers.
> >
> > I agree with you Andrea, it is a good practice to have several people
> reviewing PRs. The problem right now is that a lot of small changes are
> required to make progress on the first release. Some of these changes need
> to be in main in order to be tested (e.g. github page configuration). I
> don’t want to bother everyone with these changes and I also want to revert
> failed experiments (I know this is bad, but the history will be cleaner ;).
> >
> > I suggest we remove the branch protection rule for now and act
> responsibly when it comes to asking for reviews.
> >
> >
> > > On 15 Nov 2022, at 10:20, Bertrand Delacretaz <bd...@apache.org>
> wrote:
> > >
> > > Hi,
> > >
> > > Bertil Chapuis <bc...@gmail.com> wrote:
> > >> ...Do you think we should relax the current policy and disable the
> review requirement?...
> > >
> > > I think it's good for the project to define whether it wants to
> > > operate in CTR or RTC mode (Commit-Then-Review or Review-Then-Commit,
> > > [1])
> > >
> > > IMO declaring CTR generally (which means no pre-merge review required)
> > > and if needed RTC for some critical parts of the code works well.
> > >
> > > People are supposed to watch the commit logs and especially review
> > > changes before making releases, so in general I think CTR is fine.
> > >
> > > -Bertrand
> > >
> > > [1] https://www.apache.org/foundation/glossary.html
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@baremaps.apache.org
> > > For additional commands, e-mail: dev-help@baremaps.apache.org
> > >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@baremaps.apache.org
> For additional commands, e-mail: dev-help@baremaps.apache.org
>
> --
Sent from A Mobile Device
Re: Reviews of pull requests
Posted by Julian Hyde <jh...@apache.org>.
The "CTR vs RTC" discussion is an important one for a community to
have. In my opinion, there's no easy answer. (I'm basically agreeing
with Bertrand here.) It is certainly useful to have a default process,
and that default process should probably be RTC. But also develop
trust so that people who have gained merit can move faster. ("Just do
it" and "Small reversible steps" are ASF mantras that support that
philosophy.)
On Tue, Nov 15, 2022 at 4:42 AM Bertil Chapuis <bc...@gmail.com> wrote:
>
> Thank you Andrea and Bertrand for your answers.
>
> I agree with you Andrea, it is a good practice to have several people reviewing PRs. The problem right now is that a lot of small changes are required to make progress on the first release. Some of these changes need to be in main in order to be tested (e.g. github page configuration). I don’t want to bother everyone with these changes and I also want to revert failed experiments (I know this is bad, but the history will be cleaner ;).
>
> I suggest we remove the branch protection rule for now and act responsibly when it comes to asking for reviews.
>
>
> > On 15 Nov 2022, at 10:20, Bertrand Delacretaz <bd...@apache.org> wrote:
> >
> > Hi,
> >
> > Bertil Chapuis <bc...@gmail.com> wrote:
> >> ...Do you think we should relax the current policy and disable the review requirement?...
> >
> > I think it's good for the project to define whether it wants to
> > operate in CTR or RTC mode (Commit-Then-Review or Review-Then-Commit,
> > [1])
> >
> > IMO declaring CTR generally (which means no pre-merge review required)
> > and if needed RTC for some critical parts of the code works well.
> >
> > People are supposed to watch the commit logs and especially review
> > changes before making releases, so in general I think CTR is fine.
> >
> > -Bertrand
> >
> > [1] https://www.apache.org/foundation/glossary.html
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@baremaps.apache.org
> > For additional commands, e-mail: dev-help@baremaps.apache.org
> >
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@baremaps.apache.org
For additional commands, e-mail: dev-help@baremaps.apache.org
Re: Reviews of pull requests
Posted by Bertil Chapuis <bc...@gmail.com>.
Thank you Andrea and Bertrand for your answers.
I agree with you Andrea, it is a good practice to have several people reviewing PRs. The problem right now is that a lot of small changes are required to make progress on the first release. Some of these changes need to be in main in order to be tested (e.g. github page configuration). I don’t want to bother everyone with these changes and I also want to revert failed experiments (I know this is bad, but the history will be cleaner ;).
I suggest we remove the branch protection rule for now and act responsibly when it comes to asking for reviews.
> On 15 Nov 2022, at 10:20, Bertrand Delacretaz <bd...@apache.org> wrote:
>
> Hi,
>
> Bertil Chapuis <bc...@gmail.com> wrote:
>> ...Do you think we should relax the current policy and disable the review requirement?...
>
> I think it's good for the project to define whether it wants to
> operate in CTR or RTC mode (Commit-Then-Review or Review-Then-Commit,
> [1])
>
> IMO declaring CTR generally (which means no pre-merge review required)
> and if needed RTC for some critical parts of the code works well.
>
> People are supposed to watch the commit logs and especially review
> changes before making releases, so in general I think CTR is fine.
>
> -Bertrand
>
> [1] https://www.apache.org/foundation/glossary.html
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@baremaps.apache.org
> For additional commands, e-mail: dev-help@baremaps.apache.org
>
Re: Reviews of pull requests
Posted by Bertrand Delacretaz <bd...@apache.org>.
Hi,
Bertil Chapuis <bc...@gmail.com> wrote:
> ...Do you think we should relax the current policy and disable the review requirement?...
I think it's good for the project to define whether it wants to
operate in CTR or RTC mode (Commit-Then-Review or Review-Then-Commit,
[1])
IMO declaring CTR generally (which means no pre-merge review required)
and if needed RTC for some critical parts of the code works well.
People are supposed to watch the commit logs and especially review
changes before making releases, so in general I think CTR is fine.
-Bertrand
[1] https://www.apache.org/foundation/glossary.html
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@baremaps.apache.org
For additional commands, e-mail: dev-help@baremaps.apache.org
Re: Reviews of pull requests
Posted by Andrea Borghi <an...@gmail.com>.
Hi Bertil,
I know that I am not reviewing much these days so my voice probably doesn't
count much as I am a bit of a bottleneck potentially, but I am in principle
against a no review policy, even for trivial stuff.
The main reason being that there should be always at least 2 people aware
of some changes. The author and any reviewer in this case. Moreover, it is
easy to make stupid errors when doing "easy" things...
Cheers
On Mon, Nov 14, 2022, 21:27 Bertil Chapuis <bc...@gmail.com> wrote:
> Hi Everyone,
>
> The current policy is to have at least one review before merging. Some
> changes need to be reviewed. However, I believe that many incremental
> changes (config, cleaning, typos, etc.) can be reviewed after being merged.
> I'd like committers to feel empowered to modify and improve the code base
> when needed. Do you think we should relax the current policy and disable
> the review requirement?
>
> Best,
>
> Bertil
>