Here goes my vote with too :-P On Tue, Feb 11, 2025 at 12:37 AM Tomek CEDRO <to...@cedro.info> wrote: > 1. In Contributing Guidelines we are adding additional section for > Reviewers in order to provide complementary set of rules that should > filter out breaking code as much as possible also on our side.
+1: This will also help new reviewers, and keep good standard for everyone. > 2. Each PR (including git commits) _must_ adhere to requirements > presented in Contributing Guidelines or will be auto-rejected until > fixed / updated. +1 > 3. Git commit messages are as important as PR descriptions. These > provide in-code descriptions of each change and are git interface > independent. +1: This is good when we look at git log directly and not the web interface like github. > 4. Proper description of change is mandatory. Description must contain > explanation on what proposed change do, why it is necessary, what if > fixes, and how things are changed / fixed / updated, what is the > impact (build / runtime / api / what area), how it was tested. Local > code build and real world hardware runtime test logs must be provided > where mandatory. Description can be single..several sentences long or > bullet points but enough for anyone to understand change goals and > details. Usually it will look similar for PR and git commit message. +1: Good description is really important. It don't have to be a book, just something that will help other people understand what/why/how/where is changed. This will help reviewers. Big changes will require more information. I see that as general rule for both PR and Git commit descriptions. > 5. Proper description in PR is mandatory, or change is auto-rejected > until fixed / updated. Build and real world hardware runtime logs are > mandatory. +1: If someone develops a code, they need to build and test is anyways, so code changes should require build and runtime logs. For non code changes (i.e. documentation) there are no logs, but something that will prove someone tested stuff locally, maybe this should be clarified. This for sure will prevent situation where someone introduces a change that is not even tested locally and go live testing on ci (that may not catch everything) and other people systems / products. > 6. Proper description in Git commit message is mandatory, or change is > rejected until fixed / updates. Build and runtime logs are optional > here if these are too long and already provided in PR. +1 > 7. Each git commit message must consist of topic, description, and > signature, which are mandatory, or change is auto-rejected until fixed > / updated. Topic consists of functional prefix, ":" mark, and short > self-explanatory context. Description is separated from topic with a > single blank line. Example already presented in Contributing > Guidelines. +1 > 8. Changes must come with with documentation update where applicable. > If change presents new functionality a documentation must be provided > in the same PR (not in future). If change requires documentation > update it must be contained in the same PR (not in future). Successful > documentation build log shortcut is welcome. +1: This is a good rule, people often deliver code and documentation changes which is good, but there are places where documentation is out of sync so this should prevent code-doc desync and update missing docs on code update. > 9. We implement zero trust approach to user provided testing. It is > the commit author duty to provide real world hardware build and > runtime logs that must prove change does not break stuff for others. +1: Strong +1 here, people must realize their changes may impact others, so proper testing is required. We need to also provide tools to make that testing easier (i.e. drunx). > 10. Breaking changes are not welcome. This is anything that alters > Build / Kernel / Architecture / API, alters both nuttx and nuttx-apps > repo at the same time, breaks build/runtime/api for single or many > boards/architectures/applications, breaks self-compatibility, breaks > build/runtime compatibility with existing release code (packages) both > for nuttx and nuttx-apps, etc. +1: This is a good general rule to avoid "break by design". We do not want to change critical stuff that affects different areas in a breaking way. Sure there will be exceptions where breaking stuff may be unavoidable, but it must be well marked, noted, discussed, agreed, and tested before merge. In most cases changes can be made with alternative / complementary solutions that provide compatibility or choice. New features are not breaking changes, and should not break existing stuff. > 11. We respect long term maintenance and self-compatibility is our > ultimate goal. Alternative solutions and non-invasive approaches are > preferred that offers user a choice and compatibility. Breaking > changes are avoided, and planned towards next major release. +1: As above, breaking changes are not welcome and avoided, but sometimes unavoidable, what required good discussion prior change. We should keep self-compatibility between releases, breaking changes if happen these should be clearly noted. It may happen that in upstream master problems may occur, but these should be fixed before a release, and releases should be self-compatible. > 12. Breaking changes _must_ be discussed prior introduction on the > dev@ mailing list. PR may be created with clear indication it is for > discussion and marked as draft not to be "accidentally merged". +1 > 13. Breaking changes are special case where build and > runtime test logs (i.e. apps/ostest) from more than one different > architecture is mandatory. QEmu tests does not count here as it passed > breaking change that did not work on a real hardware. +1: Yes critical changes that are or may be breaking should be tested on various hardware architectures and logs should be provided from these tests by the author, that will help in review and at least prove someone tested it. > 14. Number of minimum required code review votes should be increased > from 2 to 4. This will ensure cross-checks and filter out faulty > changes. +1: This should ensure cross-checks and should mobilize community :-) > 15. Counting review votes should come from independent organizations. > There may be more than one review from a single organization, but > these will count as one vote. +1: Cross-checks are supposed to work that way. We should clarify affiliations of the committers. > 16. Single company commit, review, merge is not allowed. +1 > 17. Self committed code merge is not allowed. +1: This rule obeys already but is not written down? -- CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
