Skip to content

Commit 665e97e

Browse files
Erich-McMillanmakubackiErich-McMillan
authored
Add pull-request best practices guide (#70)
# Description Adds pull request best practices to CONTRIBUTING.md and references this document + pull request best practices in the pull_request_template.md so all contributors are confronted with this guide before they open a pull request rather than afterward by the bot. Co-authored-by: Michael Kubacki <[email protected]> Co-authored-by: Erich-McMillan <[email protected]>
1 parent 26378ff commit 665e97e

File tree

2 files changed

+52
-0
lines changed

2 files changed

+52
-0
lines changed

.sync/github_templates/contributing/CONTRIBUTING.md

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,52 @@ If you cannot find an existing issue that describes your bug or feature, create
4646
Please continue to follow your request after it is submitted to assist with any additional information that might be
4747
requested.
4848

49+
### Pull Request Best Practices
50+
51+
Pull requests for UEFI code can become large and difficult to review due to the large number of build and
52+
configuration files. To aid maintainers in reviewing your code, we suggest adhering to the following guidelines:
53+
54+
1. Do keep code reviews single purpose; don't add more than one feature at a time.
55+
2. Do fix bugs independently of adding features.
56+
3. Do provide documentation and unit tests.
57+
4. Do introduce code in digestible amounts.
58+
* If the contribution logically be broken up into separate pull requests that independently build and function
59+
successfully, do use multiple pull requests.
60+
61+
#### Code Categories
62+
63+
To keep code digestible, you may consider breaking large pull requests into three categories of commits within the pull
64+
request.
65+
66+
1. **Interfaces**: .h, .inf, .dec, documentation
67+
2. **Implementation**: .c, unit tests, unit test build file; unit tests should build and run at this point
68+
3. **Integration/Build**: .dec, .dsc, .fdf, (.yml) configuration files, integration tests; code added to platform and
69+
affects downstream consumers
70+
71+
By breaking the pull request into these three categories, the pull request reviewers can digest each piece
72+
independently.
73+
74+
If your commits are still very large after adhering to these categories, consider further breaking the pull request
75+
down by library/driver; break each component into its own commit.
76+
77+
#### Implementation Limits
78+
79+
Implementation is ultimately composed of functions as logical units of code.
80+
81+
To help maintainers review the code and improve long-term maintainability, limit functions to 60 lines of code. If your
82+
function exceeds 60 lines of code, it likely has also exceeded a single responsibility and should be broken up.
83+
84+
Files are easier to review and maintain if they contain functions that serves similar purpose. Limit files to around
85+
1,000 lines of code (excluding comments). If your file exceeds 1,000 lines of code, it may have functions that should
86+
be split into separate files.
87+
88+
---
89+
90+
By following these guidelines, your pull requests will be reviewed faster, and you'll avoid being asked to refactor the
91+
code to follow the guidelines.
92+
93+
Feel free to create a draft pull request and ask for suggestions on how to split the pull request if you are unsure.
94+
4995
## Thank You
5096

5197
Thank you for your interest in Project Mu and taking the time to contribute!

.sync/github_templates/pull_requests/pull_request_template.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,9 @@
1+
# Preface
2+
3+
Please ensure you have read the [contribution docs](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md) prior
4+
to submitting the pull request. In particular,
5+
[pull request guidelines](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md#pull-request-best-practices).
6+
17
## Description
28

39
<_Please include a description of the change and why this change was made._>

0 commit comments

Comments
 (0)