Hi all, I have read the workflow in the code of Bedevere (https://github.com/python/bedevere/blob/master/bedevere/stage.py#L7), and there is one thing I do not understand. Here is the PR - https://github.com/python/cpython/pull/5132 Steps: 1. label: "awaiting review" 2. I did the review and ask for some changes, so in this case, I don't approve the review. 3. the label has been changed to "awaiting core review" by Bedevere, but Why? I requested some changes and the PR is not in "awaiting changes" For me, normally, the label should be on "awaiting changes" and after a new commit/message from the author, the label should be "awaiting review" Thank you, Stéphane -- Stéphane Wirtel - https://wirtel.be - @matrixise
On Tue, May 15, 2018 at 6:12 PM, Stephane Wirtel <stephane@wirtel.be> wrote:
For me, normally, the label should be on "awaiting changes" and after a new commit/message from the author, the label should be "awaiting review"
The reviewer wasn't a core developer so we need to get a approval/review from a core developer before moving to the next step (for example, a core developer may disagree with the non-core reviewer's comments or suggest a different approach) --Berker
On Tue, 15 May 2018 at 13:03 Berker Peksağ <berker.peksag@gmail.com> wrote:
On Tue, May 15, 2018 at 6:12 PM, Stephane Wirtel <stephane@wirtel.be> wrote:
For me, normally, the label should be on "awaiting changes" and after a new commit/message from the author, the label should be "awaiting review"
The reviewer wasn't a core developer so we need to get a approval/review from a core developer before moving to the next step (for example, a core developer may disagree with the non-core reviewer's comments or suggest a different approach)
And the reason we want a core review is we have no idea if the review by a non-core reviewer is reasonable. E.g. someone might be extremely pedantic about PEP 8 when a core dev wouldn't be in all situations, so holding up a PR for that wouldn't be fair for the PR submitter.
On 05/16, Brett Cannon wrote:
On Tue, 15 May 2018 at 13:03 Berker Peksağ <berker.peksag@gmail.com> wrote:
On Tue, May 15, 2018 at 6:12 PM, Stephane Wirtel <stephane@wirtel.be> wrote:
For me, normally, the label should be on "awaiting changes" and after a new commit/message from the author, the label should be "awaiting review"
The reviewer wasn't a core developer so we need to get a approval/review from a core developer before moving to the next step (for example, a core developer may disagree with the non-core reviewer's comments or suggest a different approach)
And the reason we want a core review is we have no idea if the review by a non-core reviewer is reasonable. E.g. someone might be extremely pedantic about PEP 8 when a core dev wouldn't be in all situations, so holding up a PR for that wouldn't be fair for the PR submitter. +1 I do understand and make sense
Is there an intermediate status, like 'non-core reviewer' where the reviewer could requests changes on a PR and keep the status 'await changes' or 'await review'? By the way, maybe we could increase the number of reviewers and improve the quality of the reviews. The core dev would be more "confident" with the reviews from a 'reviewer'. For the rewiews of code, the workflow could be this one "Contributor" -> "Reviewer" -> "Core-Dev" ? -- Stéphane Wirtel - http://wirtel.be - @matrixise
On Wed, 23 May 2018 at 06:59 Stephane Wirtel <stephane@wirtel.be> wrote:
On 05/16, Brett Cannon wrote:
On Tue, 15 May 2018 at 13:03 Berker Peksağ <berker.peksag@gmail.com> wrote:
On Tue, May 15, 2018 at 6:12 PM, Stephane Wirtel <stephane@wirtel.be> wrote:
For me, normally, the label should be on "awaiting changes" and after a new commit/message from the author, the label should be "awaiting review"
The reviewer wasn't a core developer so we need to get a approval/review from a core developer before moving to the next step (for example, a core developer may disagree with the non-core reviewer's comments or suggest a different approach)
And the reason we want a core review is we have no idea if the review by a non-core reviewer is reasonable. E.g. someone might be extremely pedantic about PEP 8 when a core dev wouldn't be in all situations, so holding up a PR for that wouldn't be fair for the PR submitter. +1 I do understand and make sense
Is there an intermediate status, like 'non-core reviewer' where the reviewer could requests changes on a PR and keep the status 'await changes' or 'await review'?
By the way, maybe we could increase the number of reviewers and improve the quality of the reviews. The core dev would be more "confident" with the reviews from a 'reviewer'.
For the rewiews of code, the workflow could be this one "Contributor" -> "Reviewer" -> "Core-Dev" ?
It would require creating a list somewhere of GitHub usernames that Bedevere can read from to determine who the "reviewers" are whose reviews we trust. Then again, if someone gets that far we might want to just give them commit rights at that point. -Brett
-- Stéphane Wirtel - http://wirtel.be - @matrixise
On 05/23, Brett Cannon wrote:
On Wed, 23 May 2018 at 06:59 Stephane Wirtel <stephane@wirtel.be> wrote:
On 05/16, Brett Cannon wrote:
On Tue, 15 May 2018 at 13:03 Berker Peksağ <berker.peksag@gmail.com> wrote:
On Tue, May 15, 2018 at 6:12 PM, Stephane Wirtel <stephane@wirtel.be> wrote:
For me, normally, the label should be on "awaiting changes" and after a new commit/message from the author, the label should be "awaiting review"
The reviewer wasn't a core developer so we need to get a approval/review from a core developer before moving to the next step (for example, a core developer may disagree with the non-core reviewer's comments or suggest a different approach)
And the reason we want a core review is we have no idea if the review by a non-core reviewer is reasonable. E.g. someone might be extremely pedantic about PEP 8 when a core dev wouldn't be in all situations, so holding up a PR for that wouldn't be fair for the PR submitter. +1 I do understand and make sense
Is there an intermediate status, like 'non-core reviewer' where the reviewer could requests changes on a PR and keep the status 'await changes' or 'await review'?
By the way, maybe we could increase the number of reviewers and improve the quality of the reviews. The core dev would be more "confident" with the reviews from a 'reviewer'.
For the rewiews of code, the workflow could be this one "Contributor" -> "Reviewer" -> "Core-Dev" ?
It would require creating a list somewhere of GitHub usernames that Bedevere can read from to determine who the "reviewers" are whose reviews we trust. Then again, if someone gets that far we might want to just give them commit rights at that point.
-Brett
Maybe some contributors would be interested by this status of "reviewers". Sorry for the delay, the last week I was too busy with my job :/ -- Stéphane Wirtel - http://wirtel.be - @matrixise
On 06/05, Stephane Wirtel wrote:
On 05/23, Brett Cannon wrote:
On Wed, 23 May 2018 at 06:59 Stephane Wirtel <stephane@wirtel.be> wrote:
On 05/16, Brett Cannon wrote:
On Tue, 15 May 2018 at 13:03 Berker Peksağ <berker.peksag@gmail.com> wrote:
On Tue, May 15, 2018 at 6:12 PM, Stephane Wirtel <stephane@wirtel.be> wrote:
For me, normally, the label should be on "awaiting changes" and after a new commit/message from the author, the label should be "awaiting review"
The reviewer wasn't a core developer so we need to get a approval/review from a core developer before moving to the next step (for example, a core developer may disagree with the non-core reviewer's comments or suggest a different approach)
And the reason we want a core review is we have no idea if the review by a non-core reviewer is reasonable. E.g. someone might be extremely pedantic about PEP 8 when a core dev wouldn't be in all situations, so holding up a PR for that wouldn't be fair for the PR submitter. +1 I do understand and make sense
Is there an intermediate status, like 'non-core reviewer' where the reviewer could requests changes on a PR and keep the status 'await changes' or 'await review'?
By the way, maybe we could increase the number of reviewers and improve the quality of the reviews. The core dev would be more "confident" with the reviews from a 'reviewer'.
For the rewiews of code, the workflow could be this one "Contributor" -> "Reviewer" -> "Core-Dev" ?
It would require creating a list somewhere of GitHub usernames that Bedevere can read from to determine who the "reviewers" are whose reviews we trust. Then again, if someone gets that far we might want to just give them commit rights at that point.
-Brett
Maybe some contributors would be interested by this status of "reviewers".
Sorry for the delay, the last week I was too busy with my job :/ Yep, I would be interested by this status of "reviewers" and really sorry for the big delay (clients, vacations, europython, etc...)
that will be the same for the Python triagers if we migrate the bug tracker from b.p.o to github. Stéphane
On 05/15, Berker Peksağ wrote:
On Tue, May 15, 2018 at 6:12 PM, Stephane Wirtel <stephane@wirtel.be> wrote:
For me, normally, the label should be on "awaiting changes" and after a new commit/message from the author, the label should be "awaiting review"
The reviewer wasn't a core developer so we need to get a approval/review from a core developer before moving to the next step (for example, a core developer may disagree with the non-core reviewer's comments or suggest a different approach)
--Berker Hi Berker,
Sorry for this late reply but I was busy just after the PyCon US and the CPython sprints. Thanks for your explanation. Stéphane
_______________________________________________ core-workflow mailing list -- core-workflow@python.org To unsubscribe send an email to core-workflow-leave@python.org https://mail.python.org/mm3/mailman3/lists/core-workflow.python.org/ This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct
-- Stéphane Wirtel - http://wirtel.be - @matrixise
participants (3)
-
Berker Peksağ -
Brett Cannon -
Stephane Wirtel