Attention is currently required from: Alexander Goncharov, Angel Pons, Edward O'Callaghan, Nikolai Artemiev, Peter Marheine, Stefan Reinauer, Swift Geek (Sebastian Grzywna), Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75906?usp=email )
Change subject: doc: Add new Development guidelines ......................................................................
Patch Set 5:
(6 comments)
File doc/dev_guide/development_guidelines.rst:
https://review.coreboot.org/c/flashrom/+/75906/comment/2571ede7_8a0db6e7 : PS2, Line 7: Set up the git repository and dev environment
+1, having detailed instructions in a separate "how-to" document (or multiple) would make more sense […]
Renamed to development guide, I am happy to rename this doc!
But, I would really like to keep it as one doc, means not breaking down into even smaller pieces. There are already few pieces that would become separate docs, but whatever is here now I think should stay here. It is easier to read one doc, and there are links in here already to point for more information. But I think this one doc is what we want *everyone* to read, and having one doc greatly increase the chance one reads and understands it.
Would you agree with that?
https://review.coreboot.org/c/flashrom/+/75906/comment/f1c96d62_098e732c : PS2, Line 225: Code review process
This heading seems redundant because other sections also discuss code review. […]
Done
https://review.coreboot.org/c/flashrom/+/75906/comment/15176290_83872b9f : 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.
Done
https://review.coreboot.org/c/flashrom/+/75906/comment/9f51ac0f_180fe8d0 : 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 o […]
Done
https://review.coreboot.org/c/flashrom/+/75906/comment/a9010133_acda4b8f : 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"
Done
https://review.coreboot.org/c/flashrom/+/75906/comment/94e412a7_dd9d0fcd : 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".
Done