1.. _docs-code_reviews: 2 3====================== 4Code review guidelines 5====================== 6All Pigweed development happens on Gerrit, following the `typical Gerrit 7development workflow <http://ceres-solver.org/contributing.html>`_. Consult the 8`Gerrit User Guide 9<https://gerrit-documentation.storage.googleapis.com/Documentation/2.12.3/intro-user.html>`_ 10for more information on using Gerrit. 11 12You may add the special address 13``[email protected]`` as a reviewer to 14have Gerrit automatically choose an appropriate person to review your change. 15 16----------- 17For authors 18----------- 19 20.. _docs-code_reviews-small-changes: 21 22Small changes 23============= 24Please follow the guidance in `Google's Eng-Practices Small CLs 25<https://google.github.io/eng-practices/review/developer/small-cls.html>`_. 26 27Complete changes 28================ 29Please follow the guidance in :ref:`docs-contributing-contribution-standards`. 30In summary, CLs must be complete, tested, and include documentation and unit 31tests for new code, bug fixes, and any code changes that merit it. However, to 32enable iterative work and small changes, ``TODO`` comments are acceptable. They 33must include an explanation of the problem and an action to take. 34 35We will not take over incomplete changes to avoid shifting our focus. We may 36reject changes that do not meet the criteria above. 37 38------------- 39For reviewers 40------------- 41 42Review speed 43============ 44Follow the advice at `Speed of Code Reviews 45<https://google.github.io/eng-practices/review/reviewer/speed.html>`_. Most 46importantly, 47 48* If you are not in the middle of a focused task, **you should do a code review 49 shortly after it comes in**. 50* **One business day is the maximum time it should take to respond** to a code 51 review request (i.e., first thing the next morning). 52* If you will not be able to review the change within a business day, comment 53 on the change stating so, and reassign to another reviewer if possible. 54 55Attention set management 56======================== 57Remove yourself from the `attention set 58<https://gerrit-review.googlesource.com/Documentation/user-attention-set.html>`_ 59for changes where another person (author or reviewer) must take action before 60you can continue to review. You are encouraged, but not required, to leave a 61comment when doing so, especially for changes by external contributors who may 62not be familiar with our process. 63 64---------------------- 65Common advice playbook 66---------------------- 67What follows are bite-sized copy-paste-able advice when doing code reviews. 68Feel free to link to them from code review comments, too. 69 70.. _docs-code_reviews-playbook-platform-design: 71 72Shared platforms require careful design 73======================================= 74Pigweed is a platform shared by many embedded projects. This makes contributing 75to Pigweed rewarding: your change will help teams around the world! But it also 76makes contributing *hard*: 77 78* Edge cases that may not matter for one project can, and eventually will, come 79 up in another one. 80* Pigweed has many modules that can be used in isolation, but should also work 81 together, exhibiting a unified design philosophy and guiding users towards 82 safe, scalable patterns. 83 84As a result, Pigweed can't be as nimble as individual embedded projects, and 85often needs to engage in more careful design review, either in meetings with 86the core team or through :ref:`SEED-0001`. But we're committed to working 87through this with you! 88 89 90.. _docs-code_reviews-playbook-stale-changes: 91 92Stale changes 93============= 94Sometimes, a change doesn't make it out of the review process: after some 95rounds of review, there are unresolved comments from the Pigweed team, but the 96author is no longer actively working on the change. 97 98For any change that's not seen activity for 3 months, the Pigweed team will, 99 100#. `File a bug <https://issues.pigweed.dev/issues?q=status:open>`_ for the 101 feature or bug that the change was addressing, referencing the change. 102#. Mark the change Abandoned in Gerrit. 103 104This does *not* mean the change is rejected! It just indicates no further 105action on it is expected. As its author, you should feel free to reopen it when 106you have time to work on it again. 107 108Before making or sending major changes or SEEDs, please reach out in our 109`chat room <https://discord.gg/M9NSeTA>`_ or on the `mailing list 110<https://groups.google.com/forum/#!forum/pigweed>`_ first to ensure the changes 111make sense for upstream. We generally go through a design phase before making 112large changes. See :ref:`SEED-0001` for a description of this process; but 113please discuss with us before writing a full SEED. Let us know of any 114priorities, timelines, requirements, and limitations ahead of time. 115 116Gerrit for PRs 117============== 118We don't currently support GitHub pull requests. All Pigweed development takes 119place on `our Gerrit instance <https://pigweed-review.googlesource.com/>`_. 120Please resubmit your change there! 121 122See :ref:`docs-contributing` for instructions, and consult the `Gerrit User 123Guide 124<https://gerrit-documentation.storage.googleapis.com/Documentation/2.12.3/intro-user.html>`_ 125for more information on using Gerrit. 126 127.. _docs-code_reviews-incomplete-docs-changes: 128 129Docs-Only Changes Do Not Need To Be Complete 130============================================ 131Documentation-only changes should generally be accepted if they make the docs 132better or more complete, even if the documentation change itself is incomplete. 133