Attention is currently required from: Alexander Goncharov, Anastasia Klimchuk, Angel Pons, Edward O'Callaghan, Stefan Reinauer, Swift Geek (Sebastian Grzywna), Thomas Heijligen.
10 comments:
File doc/dev_guide/development_guidelines.rst:
Patch Set #2, 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?
Patch Set #2, 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``).
Patch Set #2, Line 141: there
Remove unnecessary word.
Patch Set #2, 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.
sending
it to review.coreboot.org.
Link this to the Working With Gerrit heading above?
Patch Set #2, 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.
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.
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.
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"
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".
To view, visit change 75906. To unsubscribe, or for help writing mail filters, visit settings.