-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Read the Docs autobuild to contributors guide #26
Conversation
some formatting and typos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just found one typo.
Co-authored-by: Kevin Paul <[email protected]>
Co-authored-by: Anderson Banihirwe <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Should we merge this? Or should we wait for a few more reviews? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I think it's good to merge.
I want to take this opportunity to revisit our PR approval discussion. We agreed that a minimum of 2 reviewers are required for approval. I think we also agreed that it is important to try and streamline the process to both speed turnaround and reduce our load (we all can't review every PR). Though we didn't pick a number, this desire effectively puts an upper bound on the number of reviewers. We also agreed - and I think the IWG has already implemented - to put in triggers that would automatically tag the appropriate folks for reviews. This PR has 8 reviewers assigned to it, which IMHO is on the too many side of the spectrum. More important than how many is who. Are the right people looking at a particular change? Perhaps with some tweaking we can automate the auto-selection of reviewers so the right people (based on their subject areas expertise) and right number are being assigned. An alternative approach that we've used on other projects is to have a project manager make assignments based on the nature of the change. Sometimes the initial assignments are wrong and it's up to the assignees to alert the project manager, or simply request reviews from other contributors. This approach has worked well for us in practice, but admittedly we've used it more for software development than a project like Pythia. Thoughts on this? |
Good points @clyne I don't know who would have been the best reviewers for this particular PR. I started with everyone who was engaged on the issue referenced (#19) but then I wanted to be inclusive. I did not expect all 8 people to review it, but I figured this way at least 2 of the 8 would get to look at it faster. Perhaps you think my first instinct was best. Who do you think would have been appropriate reviewers for this PR? |
I didn't mean to pick on your PR @jukent. The last few PRs have all had long lists of reviewers assigned. I was just raising the point that if we want to be both efficient with our limited time, while ensuring we have a good product, we're going to have to think about this a little more. As to who should be assigned to your PR I don't know the answer :-). I really think it should be the job of a "content owner" to assign reviewers, not the submitter, and we haven't really designated content owners. |
I know :) This is the practice I've seen on a lot of PRs, and I think you bring up a good point. I genuinely thought we could use this PR as an example discussion of who to request from. I suppose it is a complicated conversation. Maybe we discuss in our next meeting? |
PR #23 was meant to be a first attempt at getting volunteers for the |
cdb515d
This reverts commit cdb515d.
I'm not sure why it says I "dismissed" your "stale review" @kmpaul @andersy005 @brian-rose -- I didn't mean to dismiss anything. I accidentally pushed to this branch and then reverted it, I think that is connected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks even better now.
Let's give it a try and see how that works. I've added myself to content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @jukent!
Ready to merge? |
Related to #19