Difference between revisions of "DaalaReview"

From XiphWiki
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) breaks the build, it gets fixed immediately or removed
+
** 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 "obviously correct" or that is trivially testable gets post-review
+
*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) applies)
+
*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 ==
1) da726b32, 53362b77
+
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=da726b32 da726b32], [https://git.xiph.org/?p=daala.git;a=commitdiff;h=53362b77 53362b77]
2) 212d13b3,  
+
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=212d13b3 212d13b3],  
3) 76ee32c8, 75b62b23
+
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=76ee32c8 76ee32c8], [https://git.xiph.org/?p=daala.git;a=commitdiff;h=75b62b23 75b62b23]
4) 7c102188, 50732d66
+
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=7c102188 7c102188], [https://git.xiph.org/?p=daala.git;a=commitdiff;h=50732d66 50732d66]
5) 8cd46564, 74dc3616
+
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=8cd46564 8cd46564], [https://git.xiph.org/?p=daala.git;a=commitdiff;h=74dc3616 74dc3616]
6) 208ea300
+
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=208ea300 208ea300]
7) b4bfee65
+
* [https://git.xiph.org/?p=daala.git;a=commitdiff;h=b4bfee65 b4bfee65]
8) 7cc3cc5f, 487fe063
+
* [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 16: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

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)