Hi Owen - I wasn't aware that non-committers can't add reviewers to their PRs
but I don't see how using DRAFT mode helps with that. The idea that I can't
request a review until the commit checks all pass seems absurd to me.
On 10/28/20, 9:15 AM, "Owen Nichols" <onich...@vmware.com> wrote:
Hey Bruce, please consider that non-committers are not permitted to add
reviewers themselves, so a consistent convention to indicate when a PR has
moved from work-in-progress to ready-for-review will help alert the community
when to assign reviewers.
Currently, I see countless creative variations on people adding "WIP" or
"DO NOT REVIEW" or other text somewhere in the summary or description. I think
the suggestion here is to standardize on using Draft status as the canonical
way to communicate this.
Also worth noting, you can change a PR back to 'draft' mode at any time (so
whether you forgot initially, or a test failure or review feedback made you
realize more work is needed, you can always amend the draft status to
communicate whether you're still working, or ready for review again).
I'm not sure why you think this is going to make it more difficult to
attract and retain new contributors. The point is to make it easier for new
contributors to get their PRs reviewed, and give reviewers more confident that
their time is appreciated.
I see your point that as a committer, you have no need to communicate this
status, since you can simply add reviewers yourself when you're ready. Asking
everyone to use draft status is not the ideal workaround for the GitHub flaw
that non-committers can't request reviewers, but it also takes only 2 seconds
of your time. Expecting new contributors to email the dev list every time they
want to request a review of their PR seems far more problematic in term of
attracting and retaining new contributors.
On 10/28/20, 8:10 AM, "Bruce Schuchardt" <bru...@vmware.com> wrote:
-1
While I often use the Draft option I don't see why we want to add even
more rules about how we use github. I think it's enough to put in a PR and
then add reviewers when you're ready for comments. Getting the stink-eye for
putting up a non-Draft PR is just going to make it more difficult to attract
and retain new contributors.
On 10/27/20, 5:41 PM, "Udo Kohlmeyer" <u...@vmware.com> wrote:
Dear Apache Geode Devs,
It is really great going through all the PRs that been submitted.
As Josh Long is known to say: "I work for PRs".
Whilst going through some of the PRs I do see that there are many
PRs that have multiple commits against the PR.
I know that the PR submission framework kicks off more testing than
we do on our own local machines. It is extremely uncommon to submit a PR the
first time and have all tests go green. Which h means we invariably iterate
over commits to make the build go green.
In this limbo time period, it is hard for any reviewer to know when
the ticket is ready to be reviewed.
I want to propose that when submitting a PR, it is initially
submitted as a DRAFT PR. This way, all test can still be run and work can be
done to make sure "green" is achieved. Once "green" status has been achieved,
the draft can then be upgraded to a final PR by pressing the "Ready For Review"
button. At this point all commits on the branch can then once again be squashed
into a single commit.
Now project committers will now know that the PR is in a state that
it can be reviewed for completeness and functionality.
In addition, it will help tremendously helpful if anyone submitting
a PR monitors their PR for activity. If there is no activity for a few days,
please feel free to ping the Apache Geode Dev community for review. If review
is request, please prioritize some time to address the feedback, as reviewers
spend time reviewing code and getting an understanding what the code is doing.
If too much time goes by, between review and addressing the review comments,
not only does the reviewer lose context, possibly requiring them to spend time
again to understand what the code was/is supposed to do, but also possibly lose
interest, as the ticket has now become cold or dropped down the list of PRs.
There are currently many PRs that are in a cold state, where the
time between review and response has been so long, that both parties (reviewer
and submitter) have forgotten about the PR.
In the case that the reviews will take more time to address than
expected, please downgrade the PR to draft status again. At this point, it does
not mean that reviewers will not be able to help anymore, as you can request a
reviewer to help with feedback and comments, until one feels that the PR is
back in a state of final submission.
So, what I'm really asking from the Dev Community:
If you submit a PR, it would be great if you can nudge the
community if there is no review on the PR. If feedback is provided on a PR,
please address it as soon as possible. This not only will help the PR progress
faster, but it will shorten the feedback loop.
Finally, start with a DRAFT PR and then upgrade to a final
PR when you feel it is in a "good" state. If more work is required, it is ok to
downgrade back to a draft, until one feels the PR is in a good state again.
Managing the PR process in this manner, will be hugely beneficial
for all community members. As reviewers will know when a PR is ready to be
reviewed. Reviewers will also feel more engaged in this process, due to timely
response to their review comments. PR submitters will feel happier, in the
knowledge that the code they spent so long meticulously crafting will possibly
make it into the project.
Any thoughts?
--Udo
