You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@arrow.apache.org by Anja <an...@gmail.com> on 2023/01/24 22:19:05 UTC
Norms for adopting PRs
Hi!
Sometimes, a change is really desireable for Apache Arrow, but the original
author has not responded to a ping, or made progress in a while. In that
case, it would be good to set expectations for PR authors that their PR
might be adopted, and set norms for credit attribution.
I wanted to start a conversation about this. The full conversation might be
more appropriate for the sync call.
I could only find one reference to PR adoption, and it was in the Reviewing
Contributions section under Social Aspects:
https://arrow.apache.org/docs/developers/reviewing.html#social-aspects
> If the contribution is genuinely desirable and the contributor is not
making any progress, it is also possible to take it up. Out of politeness,
it is however better to ask the contributor first.
and slightly related
> If a PR is generally ready for merge apart from trivial or
uncontroversial concerns, the reviewer may decide to push changes
themselves to the PR instead of asking the contributor to make the changes.
A new contributor might not look at this section of the documentation,
because it is written for reviewers, not PR authors.
Things to consider are:
* Would it make sense to mention that a PR could be adopted, and the
circumstances in which it might be adopted, in the Lifecycle of a PR:
https://arrow.apache.org/docs/developers/guide/step_by_step/pr_lifecycle.html.
* What is a decent standard for "waiting for response to ping" (1 week? 2?
Context dependent?).
* Are there community norms for credit attribution in the Squashed merge
commit, if multiple authors worked on a PR over time?
* What is the process if OP has not set "allow contributions to this PR" on
GitHub or if the adopted is not an Arrow committer? Does someone fork their
fork?
~* Anja
Re: Norms for adopting PRs
Posted by Weston Pace <we...@gmail.com>.
> What is the process if OP has not set "allow contributions to this PR" on
GitHub or if the adopted is not an Arrow committer? Does someone fork their
fork?
You cannot fork a fork. However, you can add a new remote to your
fork. This should work:
git remote add westonpace https://github.com/westonpace/arrow.git
git fetch westonpace
git checkout branch-to-finish
# make changes as normal
git push myremote branch-to-finish
Then you can open up a new PR from your personal fork. This process
should retain authorship of the original commits. However, Github
will show it as "authored by abc, committed by xyz" which is fine.
On Wed, Jan 25, 2023 at 8:01 AM Neal Richardson
<ne...@gmail.com> wrote:
>
> Correct, it does. So as long as you base your branch off of the original
> contributor's PR, or cherry-pick their commit(s) into yours, the
> attribution should show up.
>
> Neal
>
> On Wed, Jan 25, 2023 at 10:54 AM Antoine Pitrou <an...@python.org> wrote:
>
> >
> > Le 25/01/2023 à 16:47, Julian Hyde a écrit :
> > > A common practice in many Git-based projects is to add “Co-authored-by”
> > > comments. To Git they are merely lines in the commit message, but they
> > > are recognized by GitHub tooling.
> >
> > Our merge script already does that with all commit authors in the PR,
> > AFAICT.
> >
> > Regards
> >
> > Antoine.
> >
> >
> > >
> > > I merged such a commit to Calcite yesterday:
> > >
> > > a326bd2d0e0b4b6b3336f10217b0ecbb79522239.png
> > > [CALCITE-5424] Customize handling of literals based on type system ·
> > > apache/calcite@a326bd2
> > > <
> > https://github.com/apache/calcite/commit/a326bd2d0e0b4b6b3336f10217b0ecbb79522239
> > >
> > > github.com
> > > <
> > https://github.com/apache/calcite/commit/a326bd2d0e0b4b6b3336f10217b0ecbb79522239
> > >
> > >
> > > <
> > https://github.com/apache/calcite/commit/a326bd2d0e0b4b6b3336f10217b0ecbb79522239
> > >
> > >
> > > Note how GitHub says “julianhyde and olivrlee committed x hours ago”
> > > rather than the usual “olivrlee authored and julianhyde committed x
> > > hours ago”.
> > >
> > > I do this only when both authors have made substantial (> 25%)
> > > contributions. If I, as reviewer, make minor changes such as copy
> > > editing comments or adding a unit test., that does not warrant a
> > > co-author tag.
> > >
> > > Julian
> > >
> > >> On Jan 25, 2023, at 1:47 AM, Antoine Pitrou <an...@python.org> wrote:
> > >>
> > >>
> > >> Hi Anja,
> > >>
> > >> Le 24/01/2023 à 23:19, Anja a écrit :
> > >>> A new contributor might not look at this section of the documentation,
> > >>> because it is written for reviewers, not PR authors.
> > >>
> > >> That is true. Perhaps it would be desirable to split that document in
> > >> two: one with general guidelines for what makes a PR ready for
> > >> merging, one with specific guidelines for reviewers.
> > >>
> > >>> Things to consider are:
> > >>> * Would it make sense to mention that a PR could be adopted, and the
> > >>> circumstances in which it might be adopted, in the Lifecycle of a PR:
> > >>>
> > https://arrow.apache.org/docs/developers/guide/step_by_step/pr_lifecycle.html
> > .
> > >>> * What is a decent standard for "waiting for response to ping" (1
> > >>> week? 2?
> > >>> Context dependent?).
> > >>
> > >> One week or two sounds fine to me. It's certainly context-dependent
> > >> (is the contribution strongly needed, is it a prerequisite for other
> > >> changes? perhaps there's a release looming and it would be a welcome
> > >> addition? is the author usually responsive and collaborative, or did
> > >> they already abandon PRs?).
> > >>
> > >>> * Are there community norms for credit attribution in the Squashed
> > merge
> > >>> commit, if multiple authors worked on a PR over time?
> > >>
> > >> I don't think there are any. Usually I would try to keep credit to the
> > >> "main" author, which is a subjective estimate but should be correlated
> > >> to the number of lines of code changes (even if most of this code is
> > >> "boring" and someone else contributed a small critical change).
> > >>
> > >>> * What is the process if OP has not set "allow contributions to this
> > >>> PR" on
> > >>> GitHub or if the adopted is not an Arrow committer? Does someone fork
> > >>> their
> > >>> fork?
> > >>
> > >> Personally, I would create a branch on my fork with the same changes
> > >> (but with authorship potentially lost, if I just apply them as a patch).
> > >>
> > >> Regards
> > >>
> > >> Antoine.
> >
Re: Norms for adopting PRs
Posted by Neal Richardson <ne...@gmail.com>.
Correct, it does. So as long as you base your branch off of the original
contributor's PR, or cherry-pick their commit(s) into yours, the
attribution should show up.
Neal
On Wed, Jan 25, 2023 at 10:54 AM Antoine Pitrou <an...@python.org> wrote:
>
> Le 25/01/2023 à 16:47, Julian Hyde a écrit :
> > A common practice in many Git-based projects is to add “Co-authored-by”
> > comments. To Git they are merely lines in the commit message, but they
> > are recognized by GitHub tooling.
>
> Our merge script already does that with all commit authors in the PR,
> AFAICT.
>
> Regards
>
> Antoine.
>
>
> >
> > I merged such a commit to Calcite yesterday:
> >
> > a326bd2d0e0b4b6b3336f10217b0ecbb79522239.png
> > [CALCITE-5424] Customize handling of literals based on type system ·
> > apache/calcite@a326bd2
> > <
> https://github.com/apache/calcite/commit/a326bd2d0e0b4b6b3336f10217b0ecbb79522239
> >
> > github.com
> > <
> https://github.com/apache/calcite/commit/a326bd2d0e0b4b6b3336f10217b0ecbb79522239
> >
> >
> > <
> https://github.com/apache/calcite/commit/a326bd2d0e0b4b6b3336f10217b0ecbb79522239
> >
> >
> > Note how GitHub says “julianhyde and olivrlee committed x hours ago”
> > rather than the usual “olivrlee authored and julianhyde committed x
> > hours ago”.
> >
> > I do this only when both authors have made substantial (> 25%)
> > contributions. If I, as reviewer, make minor changes such as copy
> > editing comments or adding a unit test., that does not warrant a
> > co-author tag.
> >
> > Julian
> >
> >> On Jan 25, 2023, at 1:47 AM, Antoine Pitrou <an...@python.org> wrote:
> >>
> >>
> >> Hi Anja,
> >>
> >> Le 24/01/2023 à 23:19, Anja a écrit :
> >>> A new contributor might not look at this section of the documentation,
> >>> because it is written for reviewers, not PR authors.
> >>
> >> That is true. Perhaps it would be desirable to split that document in
> >> two: one with general guidelines for what makes a PR ready for
> >> merging, one with specific guidelines for reviewers.
> >>
> >>> Things to consider are:
> >>> * Would it make sense to mention that a PR could be adopted, and the
> >>> circumstances in which it might be adopted, in the Lifecycle of a PR:
> >>>
> https://arrow.apache.org/docs/developers/guide/step_by_step/pr_lifecycle.html
> .
> >>> * What is a decent standard for "waiting for response to ping" (1
> >>> week? 2?
> >>> Context dependent?).
> >>
> >> One week or two sounds fine to me. It's certainly context-dependent
> >> (is the contribution strongly needed, is it a prerequisite for other
> >> changes? perhaps there's a release looming and it would be a welcome
> >> addition? is the author usually responsive and collaborative, or did
> >> they already abandon PRs?).
> >>
> >>> * Are there community norms for credit attribution in the Squashed
> merge
> >>> commit, if multiple authors worked on a PR over time?
> >>
> >> I don't think there are any. Usually I would try to keep credit to the
> >> "main" author, which is a subjective estimate but should be correlated
> >> to the number of lines of code changes (even if most of this code is
> >> "boring" and someone else contributed a small critical change).
> >>
> >>> * What is the process if OP has not set "allow contributions to this
> >>> PR" on
> >>> GitHub or if the adopted is not an Arrow committer? Does someone fork
> >>> their
> >>> fork?
> >>
> >> Personally, I would create a branch on my fork with the same changes
> >> (but with authorship potentially lost, if I just apply them as a patch).
> >>
> >> Regards
> >>
> >> Antoine.
>
Re: Norms for adopting PRs
Posted by Antoine Pitrou <an...@python.org>.
Le 25/01/2023 à 16:47, Julian Hyde a écrit :
> A common practice in many Git-based projects is to add “Co-authored-by”
> comments. To Git they are merely lines in the commit message, but they
> are recognized by GitHub tooling.
Our merge script already does that with all commit authors in the PR,
AFAICT.
Regards
Antoine.
>
> I merged such a commit to Calcite yesterday:
>
> a326bd2d0e0b4b6b3336f10217b0ecbb79522239.png
> [CALCITE-5424] Customize handling of literals based on type system ·
> apache/calcite@a326bd2
> <https://github.com/apache/calcite/commit/a326bd2d0e0b4b6b3336f10217b0ecbb79522239>
> github.com
> <https://github.com/apache/calcite/commit/a326bd2d0e0b4b6b3336f10217b0ecbb79522239>
>
> <https://github.com/apache/calcite/commit/a326bd2d0e0b4b6b3336f10217b0ecbb79522239>
>
> Note how GitHub says “julianhyde and olivrlee committed x hours ago”
> rather than the usual “olivrlee authored and julianhyde committed x
> hours ago”.
>
> I do this only when both authors have made substantial (> 25%)
> contributions. If I, as reviewer, make minor changes such as copy
> editing comments or adding a unit test., that does not warrant a
> co-author tag.
>
> Julian
>
>> On Jan 25, 2023, at 1:47 AM, Antoine Pitrou <an...@python.org> wrote:
>>
>>
>> Hi Anja,
>>
>> Le 24/01/2023 à 23:19, Anja a écrit :
>>> A new contributor might not look at this section of the documentation,
>>> because it is written for reviewers, not PR authors.
>>
>> That is true. Perhaps it would be desirable to split that document in
>> two: one with general guidelines for what makes a PR ready for
>> merging, one with specific guidelines for reviewers.
>>
>>> Things to consider are:
>>> * Would it make sense to mention that a PR could be adopted, and the
>>> circumstances in which it might be adopted, in the Lifecycle of a PR:
>>> https://arrow.apache.org/docs/developers/guide/step_by_step/pr_lifecycle.html.
>>> * What is a decent standard for "waiting for response to ping" (1
>>> week? 2?
>>> Context dependent?).
>>
>> One week or two sounds fine to me. It's certainly context-dependent
>> (is the contribution strongly needed, is it a prerequisite for other
>> changes? perhaps there's a release looming and it would be a welcome
>> addition? is the author usually responsive and collaborative, or did
>> they already abandon PRs?).
>>
>>> * Are there community norms for credit attribution in the Squashed merge
>>> commit, if multiple authors worked on a PR over time?
>>
>> I don't think there are any. Usually I would try to keep credit to the
>> "main" author, which is a subjective estimate but should be correlated
>> to the number of lines of code changes (even if most of this code is
>> "boring" and someone else contributed a small critical change).
>>
>>> * What is the process if OP has not set "allow contributions to this
>>> PR" on
>>> GitHub or if the adopted is not an Arrow committer? Does someone fork
>>> their
>>> fork?
>>
>> Personally, I would create a branch on my fork with the same changes
>> (but with authorship potentially lost, if I just apply them as a patch).
>>
>> Regards
>>
>> Antoine.
Re: Norms for adopting PRs
Posted by Julian Hyde <jh...@gmail.com>.
A common practice in many Git-based projects is to add “Co-authored-by”
comments. To Git they are merely lines in the commit message, but they are
recognized by GitHub tooling.
I merged such a commit to Calcite yesterday:
[
---
| [[CALCITE-5424] Customize handling of literals based on type system ·
apache/calcite@a326bd2](https://github.com/apache/calcite/commit/a326bd2d0e0b4b6b3336f10217b0ecbb79522239)[github.com](https://github.com/apache/calcite/commit/a326bd2d0e0b4b6b3336f10217b0ecbb79522239)
---
](https://github.com/apache/calcite/commit/a326bd2d0e0b4b6b3336f10217b0ecbb79522239)
Note how GitHub says “julianhyde and olivrlee committed x hours ago” rather
than the usual “olivrlee authored and julianhyde committed x hours ago”.
I do this only when both authors have made substantial (> 25%) contributions.
If I, as reviewer, make minor changes such as copy editing comments or adding
a unit test., that does not warrant a co-author tag.
Julian
> On Jan 25, 2023, at 1:47 AM, Antoine Pitrou <an...@python.org> wrote:
>
>
>
> Hi Anja,
>
> Le 24/01/2023 à 23:19, Anja a écrit :
>
>
>> A new contributor might not look at this section of the documentation,
>
>
>> because it is written for reviewers, not PR authors.
>
>
>
> That is true. Perhaps it would be desirable to split that document in two:
> one with general guidelines for what makes a PR ready for merging, one with
> specific guidelines for reviewers.
>
>
>
>> Things to consider are:
>
>
>> * Would it make sense to mention that a PR could be adopted, and the
>
>
>> circumstances in which it might be adopted, in the Lifecycle of a PR:
>
>
>>
https://arrow.apache.org/docs/developers/guide/step_by_step/pr_lifecycle.html.
>
>
>> * What is a decent standard for "waiting for response to ping" (1 week? 2?
>
>
>> Context dependent?).
>
>
>
> One week or two sounds fine to me. It's certainly context-dependent (is the
> contribution strongly needed, is it a prerequisite for other changes?
> perhaps there's a release looming and it would be a welcome addition? is the
> author usually responsive and collaborative, or did they already abandon
> PRs?).
>
>
>
>> * Are there community norms for credit attribution in the Squashed merge
>
>
>> commit, if multiple authors worked on a PR over time?
>
>
>
> I don't think there are any. Usually I would try to keep credit to the
> "main" author, which is a subjective estimate but should be correlated to
> the number of lines of code changes (even if most of this code is "boring"
> and someone else contributed a small critical change).
>
>
>
>> * What is the process if OP has not set "allow contributions to this PR" on
>
>
>> GitHub or if the adopted is not an Arrow committer? Does someone fork their
>
>
>> fork?
>
>
>
> Personally, I would create a branch on my fork with the same changes (but
> with authorship potentially lost, if I just apply them as a patch).
>
> Regards
>
> Antoine.
>
Re: Norms for adopting PRs
Posted by Antoine Pitrou <an...@python.org>.
Hi Anja,
Le 24/01/2023 à 23:19, Anja a écrit :
>
> A new contributor might not look at this section of the documentation,
> because it is written for reviewers, not PR authors.
That is true. Perhaps it would be desirable to split that document in
two: one with general guidelines for what makes a PR ready for merging,
one with specific guidelines for reviewers.
> Things to consider are:
> * Would it make sense to mention that a PR could be adopted, and the
> circumstances in which it might be adopted, in the Lifecycle of a PR:
> https://arrow.apache.org/docs/developers/guide/step_by_step/pr_lifecycle.html.
>
> * What is a decent standard for "waiting for response to ping" (1 week? 2?
> Context dependent?).
One week or two sounds fine to me. It's certainly context-dependent (is
the contribution strongly needed, is it a prerequisite for other
changes? perhaps there's a release looming and it would be a welcome
addition? is the author usually responsive and collaborative, or did
they already abandon PRs?).
> * Are there community norms for credit attribution in the Squashed merge
> commit, if multiple authors worked on a PR over time?
I don't think there are any. Usually I would try to keep credit to the
"main" author, which is a subjective estimate but should be correlated
to the number of lines of code changes (even if most of this code is
"boring" and someone else contributed a small critical change).
> * What is the process if OP has not set "allow contributions to this PR" on
> GitHub or if the adopted is not an Arrow committer? Does someone fork their
> fork?
Personally, I would create a branch on my fork with the same changes
(but with authorship potentially lost, if I just apply them as a patch).
Regards
Antoine.