Skip to content

Commit 5a6ffa6

Browse files
bretgsergseven
authored andcommitted
Update code-reviews.md (prebid#3560)
* Update code-reviews.md * Update code-reviews.md * Update code-reviews.md
1 parent 7bf1d7c commit 5a6ffa6

1 file changed

Lines changed: 16 additions & 22 deletions

File tree

docs/developers/code-reviews.md

Lines changed: 16 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,33 +3,21 @@
33
## Standards
44
Anyone is free to review and comment on any [open pull requests](https://github.com/prebid/prebid-server-java/pulls).
55

6-
All pull requests must be reviewed and approved by at least one [core member](https://github.com/orgs/prebid/teams/core/members) before merge.
7-
8-
Very small pull requests may be merged with just one review if they:
9-
10-
1. Do not change the public API.
11-
2. Have low risk of bugs, in the opinion of the reviewer.
12-
3. Introduce no new features, or impact the code architecture.
13-
14-
Larger pull requests must meet at least one of the following two additional requirements.
15-
16-
1. Have a second approval from a core member
17-
2. Be open for 5 business days with no new changes requested.
6+
1. PRs that touch only adapters and modules can be approved by one reviewer before merge.
7+
2. PRs that touch PBS-core must be reviewed and approved by at least two 'core' reviewers before merge.
188

199
## Process
2010

21-
New pull requests should be [assigned](https://help.github.com/articles/assigning-issues-and-pull-requests-to-other-github-users/)
22-
to a core member for review within 3 business days of being opened.
23-
That person should either approve the changes or request changes within 4 business days of being assigned.
24-
If they're too busy, they should assign it to someone else who can review it within that timeframe.
11+
New pull requests must be [assigned](https://help.github.com/articles/assigning-issues-and-pull-requests-to-other-github-users/)
12+
to a reviewer within 5 business days of being opened. That person must either approve the changes or request changes within 5 business days of being assigned.
13+
14+
If a reviewer is too busy, they should re-assign it to someone else as soon as possible so that person has enough time to take over the review and still meet the 5-day goal. Please tag the new reviewer in the PR. If you don't know who to assign it to, use the #prebid-server-java-dev Slack channel to ask for help in re-assigning.
2515

26-
If the changes are small, that member can merge the PR once the changes are complete. Otherwise, they should
27-
assign the pull request to another member for a second review.
16+
If a reviewer is going to be unavailable for more than a few days, they should update the notes column of the duty spreadsheet or drop a note about their availability into the Slack channel.
2817

29-
The pull request can then be merged whenever the second reviewer approves, or if 5 business days pass with no farther
30-
changes requested by anybody, whichever comes first.
18+
After the review, if the PR touches PBS-core, it must be assigned to a second reviewer.
3119

32-
## Priorities
20+
## Review Priorities
3321

3422
Code reviews should focus on things which cannot be validated by machines.
3523

@@ -43,4 +31,10 @@ explaining it. Are there better ways to achieve those goals?
4331
- Does the code use any global, mutable state? [Inject dependencies](https://en.wikipedia.org/wiki/Dependency_injection) instead!
4432
- Can the code be organized into smaller, more modular pieces?
4533
- Is there dead code which can be deleted? Or TODO comments which should be resolved?
46-
- Look for code used by other adapters. Encourage adapter submitter to utilize common code.
34+
- Look for code used by other adapters. Encourage adapter submitter to utilize common code.
35+
- Specific bid adapter rules:
36+
- The email contact must work and be a group, not an individual.
37+
- Host endpoints cannot be fully dynamic. i.e. they can utilize "https://REGION.example.com", but not "https://HOST".
38+
- They cannot _require_ a "region" parameter. Region may be an optional parameter, but must have a default.
39+
- No direct use of HTTP is prohibited - *implement an existing Bidder interface that will do all the job*
40+
- If the ORTB is just forwarded to the endpoint, use the generic adapter - *define the new adapter as the alias of the generic adapter*

0 commit comments

Comments
 (0)