Skip to content
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

Added guidelines around timing #6

Merged
merged 2 commits into from
Sep 19, 2019
Merged

Added guidelines around timing #6

merged 2 commits into from
Sep 19, 2019

Conversation

orangejenny
Copy link
Contributor

This is what I got out of from today's conversation. Pinged as reviewers anyone who opened their mouth during that conversation. Any objections to saying "review within 24 hours most of the time" and allowing that this isn't set in stone?

@dimagi/team-commcare-hq @shubham1g5

Copy link
Member

@dannyroberts dannyroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I had a question that it might be worth clarifying in the text either way

README.md Outdated

In extremely rare circumstances where the author and reviewer cannot reach consensus, the review can be escalated, however this should be done in only the most extreme circumstances and should never be necessary if both the author and the reviewer are following the advice below.

Aim to review PRs within one business day, but use your judgement as to when to break this guideline. Common considerations:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 1-business-day guideline is about your first review of a PR after it's submitted, right? And I think independently also then applies to reviewing the changes someone made in response to your comments?

I.e. we don't expect all of the review, back and forth, and approval to happen in 24 hours except in the simplest of cases (where there reviewer approves without blocking comments). Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will clarify. My understanding is the same as yours.

@orangejenny orangejenny merged commit 88c38a3 into master Sep 19, 2019
@orangejenny orangejenny deleted the orangejenny-patch-1 branch September 19, 2019 18:18
@millerdev
Copy link
Contributor

Looks good. Thanks for adding to our docs.

@czue
Copy link
Member

czue commented Sep 20, 2019

👨‍💻 📖 🔍 ✔️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants