DaalaReview: Difference between revisions
Jump to navigation
Jump to search
(Created page with "This is a '''proposal''' for a more flexible review process. It is a set of guidelines for the most appropriate approach for different types of changes. Above all, use your judge...") |
|||
| (5 intermediate revisions by 4 users not shown) | |||
| Line 11: | Line 11: | ||
*1) Generally no review on comments/whitespace/style/documentation | *1) Generally no review on comments/whitespace/style/documentation | ||
** Except potentially when changing Doxygen API comments | ** Except potentially when changing Doxygen API comments | ||
*2)Post-review on code that is already badly broken, or on simple bug fixes | *2) Post-review on code that is already badly broken, or on simple bug fixes | ||
*3) No review on initial commit of new stuff that isn't being called | *3) No review on initial commit of new stuff that isn't being called | ||
** Full-review required to hook any code not reviewed because of 3 | ** Full-review required to hook any code not reviewed because of point 3 | ||
** If code in 3 | ** If code in point 3 breaks the build, it gets fixed immediately or removed | ||
** No review on new tools until they get used by other people | ** No review on new tools until they get used by other people | ||
*4) Code that is | *4) Code that is (according to the committer) highly unlikely to break things or that is trivially testable gets post-review | ||
*5) Code that may have subtle implications gets full review | *5) Code that may have subtle implications gets full review | ||
*6) Refactoring of someone else's code gets design review, followed by either post-review (or full review if requested during design review) | *6) Refactoring of someone else's code gets design review, followed by either post-review (or full review if requested during design review) | ||
*7) Disruptive work (e.g. block switching) gets both design review and full review. | *7) Disruptive work (e.g. block switching) gets both design review and full review. | ||
*8) Most build system changes get no review (unless 0 | *8) Most build system changes get no review (unless point 0 applies) | ||
*9) Post-review on test code | *9) Post-review on test code | ||
== Examples == | == Examples == | ||
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=da726b32 da726b32], [https://git.xiph.org/?p=daala.git;a=commitdiff;h=53362b77 53362b77] | |||
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=212d13b3 212d13b3], | |||
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=76ee32c8 76ee32c8], [https://git.xiph.org/?p=daala.git;a=commitdiff;h=75b62b23 75b62b23] | |||
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=7c102188 7c102188], [https://git.xiph.org/?p=daala.git;a=commitdiff;h=50732d66 50732d66] | |||
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=8cd46564 8cd46564], [https://git.xiph.org/?p=daala.git;a=commitdiff;h=74dc3616 74dc3616] | |||
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=208ea300 208ea300] | |||
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=b4bfee65 b4bfee65] | |||
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=7cc3cc5f 7cc3cc5f], [https://git.xiph.org/?p=daala.git;a=commitdiff;h=487fe063 487fe063] | |||
== Team reviews == | == Team reviews == | ||
| Line 38: | Line 38: | ||
*3) Code discussed with entire team | *3) Code discussed with entire team | ||
*4) Suggestions for improvement (with emphasis on algo) | *4) Suggestions for improvement (with emphasis on algo) | ||
[[Category:Daala]] | |||
Latest revision as of 15:16, 18 August 2015
This is a proposal for a more flexible review process. It is a set of guidelines for the most appropriate approach for different types of changes. Above all, use your judgement.
Definitions
- A) Full review: Same as current process. Can be based on Rietveld, "git format-patch", or a pull request
- B) Post-review: Code gets committed to repo first, committer nags reviewer weekly if necessary
- C) Design review: Discuss with original author (and possibly others) for the principle of a change, can (and probably should) be done even before doing the work
- D) No review: self-explanatory
Guidelines
- 0) Full-review on anything the committer has reasons to believe would be controversial, buggy, ...
- 1) Generally no review on comments/whitespace/style/documentation
- Except potentially when changing Doxygen API comments
- 2) Post-review on code that is already badly broken, or on simple bug fixes
- 3) No review on initial commit of new stuff that isn't being called
- Full-review required to hook any code not reviewed because of point 3
- If code in point 3 breaks the build, it gets fixed immediately or removed
- No review on new tools until they get used by other people
- 4) Code that is (according to the committer) highly unlikely to break things or that is trivially testable gets post-review
- 5) Code that may have subtle implications gets full review
- 6) Refactoring of someone else's code gets design review, followed by either post-review (or full review if requested during design review)
- 7) Disruptive work (e.g. block switching) gets both design review and full review.
- 8) Most build system changes get no review (unless point 0 applies)
- 9) Post-review on test code
Examples
- da726b32, 53362b77
- 212d13b3,
- 76ee32c8, 75b62b23
- 7c102188, 50732d66
- 8cd46564, 74dc3616
- 208ea300
- b4bfee65
- 7cc3cc5f, 487fe063
Team reviews
- 1) Once in a while, we should pick a piece of code
- 2) Original author cleans up code, adds comments
- 3) Code discussed with entire team
- 4) Suggestions for improvement (with emphasis on algo)