Attention is currently required from: Alexander Goncharov, Anastasia Klimchuk, Angel Pons, Edward O'Callaghan, Stefan Reinauer, Swift Geek (Sebastian Grzywna), Thomas Heijligen.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75906?usp=email )
Change subject: doc: Add new Development guidelines ......................................................................
Patch Set 2:
(10 comments)
File doc/dev_guide/development_guidelines.rst:
https://review.coreboot.org/c/flashrom/+/75906/comment/2bfd90d8_020fc00a : PS2, Line 7: Set up the git repository and dev environment This is more of a how-to than guideline, which seems a little weird given the document name. Maybe rename this file to the development guide instead?
https://review.coreboot.org/c/flashrom/+/75906/comment/5c168b7c_ac17dff8 : PS2, Line 27: Create a commit and sign-off ``git commit -s``. This is a good concise summary, but it's kind of confusing with this wording. Perhaps:
In short, commit your changes with a descriptive message and remember to sign off on the commit (``git commit -s``).
https://review.coreboot.org/c/flashrom/+/75906/comment/aeae58d0_5d59fbba : PS2, Line 141: there Remove unnecessary word.
https://review.coreboot.org/c/flashrom/+/75906/comment/acd3d905_467abb42 : PS2, Line 216: The only official repository is https://review.coreboot.org/flashrom and GitHub and GitLab are just mirrors. Slight modification for clarity:
The only official repository is https://review.coreboot.org/flashrom; GitHub and GitLab are only mirrors. **Reviewers will not look at pull requests** on mirrors.
https://review.coreboot.org/c/flashrom/+/75906/comment/f4b14f76_eb028aaf : PS2, Line 221: sending : it to review.coreboot.org. Link this to the Working With Gerrit heading above?
https://review.coreboot.org/c/flashrom/+/75906/comment/c7100ac8_ceb217b0 : PS2, Line 225: Code review process This heading seems redundant because other sections also discuss code review. I think the paragraphs here should be moved to other subsections where they're more relevant and this one deleted.
https://review.coreboot.org/c/flashrom/+/75906/comment/9ae43246_32521577 : PS2, Line 228: Our guidelines borrow heavily from `coreboot coding style : https://doc.coreboot.org/contributing/coding_style.html`_ and `Gerrit : guidelines https://doc.coreboot.org/contributing/gerrit_guidelines.html`_, : and most of them apply to flashrom as well. The really important part is about : the :ref:`sign-off procedure <sign-off>`. : : We try to **reuse as much code as possible** and create new files only if : absolutely needed, so if you find a function somewhere in the tree which : already does what you want, please use it. These paragraphs seems like they belong in the Coding Style section above.
https://review.coreboot.org/c/flashrom/+/75906/comment/dbc16486_a684e309 : PS2, Line 241: If you introduce new features (not flash chips, but stuff like partial : programming, support for new external programmers, voltage handling, etc) : please **discuss your plans** on the :ref:`mailing list` first. That way, we : can avoid duplicated work and know about how flashrom internals need to be : adjusted and you avoid frustration if there is some disagreement about the : design. This seems like it belongs near the top (before mechanics of how to write and share patches) since otherwise the document flow seems to suggest writing a patch but then this says you shouldn't necessarily start by just sending a patch.
https://review.coreboot.org/c/flashrom/+/75906/comment/75fa5346_16c30d64 : PS2, Line 248: Initial review should include a broad indication of acceptance or rejection of : the idea/rationale/motivation or the implementation : : In general, reviews should focus on the architectural changes and things that : affect flashrom as a whole. This includes (but is by no means limited to) : changes in APIs and types, safety, portability, extensibility, and : maintainability. The purpose of reviews is not to create perfect patches, but : to steer development in the right direction and produce consensus within the : community. The goal of each patch should be to improve the state of the project : - it does not need to fix all problems of the respective field perfectly. : : .. note:: : : New contributors may need more detailed advices and should be told about : minor issues like formatting problems more precisely. The result of a review : should either be an accepted patch or a guideline how the existing code : should be changed to be eventually accepted. : : If you sent a patch and later lost interest or no longer have time to follow up on code review, : please add a comment saying so. Then, if any of our maintainers are interested in finishing the work, : they can take over the patch. Looks like part of "Going Through Code Reviews"
https://review.coreboot.org/c/flashrom/+/75906/comment/5b055647_795506d6 : PS2, Line 270: Merging patches : =============== : : Merging to branches is limited to the "flashrom developers" group on Gerrit. : The following rules apply, some are already : enforced by Gerrit: : : * Every commit has to be reviewed and needs at least one +2 that was not given : by the commit's author. : * Except, if a commit is authored by more than one person, each author may +2 : the other author's changes. : * flashrom developers are people from literally all around the planet, and various : timezones, so we usually wait for 3 days after the patch is fully approved just in case : of last minute concerns from all timezones. : * In the case of emergency, merging should not take place within less than 24 hours after the review : started (i.e. the first message by a reviewer on Gerrit). : * Finally, before hitting Submit, one is reponsible to check that all comments : have been addressed, especially if there was a negative review (-1). Make this the final subsection of "Working With Gerrit".
-
Peter Marheine (Code Review)