You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Each commit should be a **single complete** change.
106
106
This discipline is important when reviewing the changes as well as when using `git bisect` and `git revert`.
107
107
108
-
#### Pull request submission
108
+
#### Pull request - Submission
109
109
110
110
**Always create a pull request to the `master` branch of this repository**.
111
111
112
112
113
113
114
+
* It's recommended to avoid a PR with too many changes.
115
+
A large PR not only stretches the review time, but also makes it much harder to spot issues.
116
+
In such case, it's better to split the PR to multiple smaller ones.
117
+
For large features, try to approach it in an incremental way, so that each PR won't be too big.
114
118
* If you're contributing in a way that changes the user or developer experience, you are expected to document those changes.
115
119
See [Contributing to documentation related to PowerShell](#contributing-to-documentation-related-to-powershell).
116
120
* Add a meaningful title of the PR describing what change you want to check in.
117
-
Don't simply put: "Fixes issue #5".
118
-
A better example is: "Add Ensure parameter to New-Item cmdlet", with "Fixes #5" in the PR's body.
121
+
Don't simply put: "Fix issue #5".
122
+
Also don't directly use the issue title as the PR title.
123
+
An issue title is to briefly describe what is wrong, while a PR title is to briefly describe what is changed.
124
+
A better example is: "Add Ensure parameter to New-Item cmdlet", with "Fix #5" in the PR's body.
119
125
* When you create a pull request,
120
-
including a summary of what's included in your changes and
121
-
if the changes are related to an existing GitHub issue,
122
-
please reference the issue in pull request description (e.g. ```Closes #11```).
126
+
including a summary about your changes in the PR description.
127
+
The description is used to create change logs,
128
+
so try to have the first sentence explain the benefit to end users.
129
+
If the changes are related to an existing GitHub issue,
130
+
please reference the issue in PR description (e.g. ```Fix #11```).
123
131
See [this][closing-via-message] for more details.
124
132
* If the change warrants a note in the [changelog](../CHANGELOG.MD)
125
133
either update the changelog in your pull request or
@@ -136,8 +144,8 @@ Additional references:
136
144
```
137
145
138
146
* Please use the present tense and imperative mood when describing your changes:
139
-
* Instead of "Adding support for Windows Server 2012 R2", write "Add support for Windows Server 2012 R2".
140
-
* Instead of "Fixed for server connection issue", write "Fix server connection issue".
147
+
* Instead of "Adding support for Windows Server 2012 R2", write "Add support for Windows Server 2012 R2".
148
+
* Instead of "Fixed for server connection issue", write "Fix server connection issue".
141
149
142
150
This form is akin to giving commands to the code base
143
151
and is recommended by the Git SCM developers.
@@ -167,50 +175,55 @@ Additional references:
167
175
[run the spellchecker command line tool in interactive mode](#spellchecking-documentation)
168
176
to add words to the `.spelling` file.
169
177
170
-
#### Pull Request - Code Review
171
-
172
-
* Roles and Responsibilities of a PR: Author, Reviewer, and Assignee
173
-
* Reviewer and Assignee are two separate roles of a PR.
174
-
* A Reviewer can be anyone who wants to contribute.
175
-
A Reviewer reviews the change of a PR,
176
-
leaves comments for the Author to address,
177
-
and approves the PR when the change looks good.
178
-
* An Assignee must be a [Maintainer](../docs/maintainers), who monitors the progress of the PR,
179
-
coordinates the review process, and merges the PR after it's been approved.
180
-
The Assignee may or may not be a Reviewer of the PR at the same time.
181
-
* An Author is encouraged to choose Reviewer(s) and an Assignee for the PR.
182
-
If no Assignee is chosen, one of the Maintainers shall be assigned to it.
183
-
If no Reviewer is chosen, the Assignee shall choose Reviewer(s) as appropriate.
184
-
* If an Author is a [PowerShell Team](https://github.com/orgs/PowerShell/people) member,
185
-
then the Author **is required** to choose Reviewer(s) and an Assignee for the PR.
186
-
* For a PR to be merged, it must be approved by at least one PowerShell Team member or Collaborator,
187
-
so additional Reviewer(s) may be added by the Assignee as appropriate.
188
-
The Assignee may also be re-assigned by Maintainers.
189
-
* A Reviewer can postpone the code review if CI builds fail,
190
-
but also can start the code review early regardless of the CI builds.
191
-
* The Author **is responsible** for driving the PR to the Approved state.
192
-
The Author addresses review comments, and pings Reviewer(s) to start the next iteration.
193
-
If the review is making no progress (or very slow),
194
-
the Author can always ask the Assignee to help coordinate the process and keep it moving.
195
-
* Additional feedback is always welcome!
196
-
Even if you are not designated as a Reviewer,
197
-
feel free to review others' pull requests anyway.
198
-
Leave your comments even if everything looks good;
199
-
a simple "Looks good to me" or "LGTM" will suffice.
200
-
This way we know someone has already taken a look at it!
201
-
* When updating your pull request, please **create new commits**
202
-
and **don't rewrite the commits history**. This way it's very easy for
203
-
the reviewers to see diff between iterations.
204
-
If you rewrite the history in the pull request, review could be much slower.
205
-
Once the review is done, you can rewrite the history to make it prettier,
206
-
if you like.
207
-
Otherwise it's likely would be squashed on merge to master.
208
-
* Once the code review is done,
209
-
all merge conflicts are resolved,
210
-
and the CI system build status is passing,
211
-
the PR Assignee will merge your changes.
212
-
* For more information on the PowerShell Maintainers' process,
213
-
see the [documentation](../docs/maintainers).
178
+
#### Pull Request - Workflow
179
+
180
+
1. The PR *author* creates a pull request from a fork.
181
+
1. The *author* ensures that their pull request passes the [CI system][ci-system] build.
182
+
- If the build fails, a [Repository Maintainer][repository-maintainer] adds the `Review - waiting on author` label to the pull request.
183
+
The *author* can then continue to update the pull request until the build passes.
184
+
1. If the *author* knows whom should participate in the review, they should add them otherwise they can add the recommended *reviewers*.
185
+
1. Once the build passes, if there is not sufficient review, the *maintainer* adds the `Review - needed` label.
186
+
1. An [Area Expert][area-expert] should also review the pull request.
187
+
- If the *author* does not meet the *reviewer*'s standards, the *reviewer* makes comments. A *maintainer* then removes the `Review - needed` label and adds
188
+
the `Review - waiting on author` label. The *author* must address the comments and repeat from step 2.
189
+
- If the *author* meets the *reviewer*'s standards, the *reviewer* approves the PR. A maintainer then removes the `need review` label.
190
+
1. Once the code review is completed, a *maintainer* merges the pull request after one business day to allow for additional critical feedback.
191
+
192
+
#### Pull Request - Roles and Responsibilities
193
+
194
+
1. The PR *author* is responsible for moving the PR forward to get it Approved.
195
+
This includes addressing feedback within a timely period and indicating feedback has been addressed by adding a comment and mentioning the specific *reviewers*.
196
+
When updating your pull request, please **create new commits** and **don't rewrite the commits history**.
197
+
This way it's very easy for the reviewers to see diff between iterations.
198
+
If you rewrite the history in the pull request, review could be much slower.
199
+
The PR is likely to be squashed on merge to master by the *assignee*.
200
+
1.*Reviewers* are anyone who wants to contribute.
201
+
They are responsible for ensuring the code: addresses the issue being fixed, does not create new issues (functional, performance, reliability, or security), and implements proper design.
202
+
*Reviewers* should use the `Review changes` drop down to indicate they are done with their review.
203
+
-`Request changes` if you believe the PR merge should be blocked if your feedback is not addressed,
204
+
-`Approve` if you believe your feedback has been addressed or the code is fine as-is, it is customary (although not required) to leave a simple "Looks good to me" (or "LGTM") as the comment for approval.
205
+
-`Comment` if you are making suggestions that the *author* does not have to accept.
206
+
Early in the review, it is acceptable to provide feedback on coding formatting based on the published [Coding Guidelines](../docs/dev-process/coding-guidelines.md), however,
207
+
after the PR has been approved, it is generally _not_ recommended to focus on formatting issues unless they go against the [Coding Guidelines](../docs/dev-process/coding-guidelines.md).
208
+
Non-critical late feedback (after PR has been approved) can be submitted as a new issue or new pull request from the *reviewer*.
209
+
1.*Assignee* who are always *Maintainers* ensure that proper review has occurred and if they believe one approval is not sufficient, the *maintainer* is responsible to add more reviewers.
210
+
An *assignee* may also be a reviewer, but the roles are distinct.
211
+
Once the PR has been approved and the CI system is passing, the *assignee* will merge the PR after giving one business day for any critical feedback.
212
+
For more information on the PowerShell Maintainers' process, see the [documentation](../docs/maintainers).
213
+
214
+
#### Pull Requests - Abandoned
215
+
216
+
A pull request with the label `Review - waiting on author` for **more than two weeks** without a word from the author is considered abandoned.
217
+
218
+
In these cases:
219
+
220
+
1.*Assignee* will ping the author of PR to remind them of pending changes.
221
+
- If the *author* responds, it's no longer an abandoned; the pull request proceeds as normal.
222
+
1. If the *author* does not respond **within a week**:
223
+
- If the *reviewer*'s comments are very minor, merge the change, fix the code immediately, and create a new PR with the fixes addressing the minor comments.
224
+
- If the changes required to merge the pull request are significant but needed, *assignee* creates a new branch with the changes and open an issue to merge the code into the dev branch.
225
+
Mention the original pull request ID in the description of the new issue and close the abandoned pull request.
226
+
- If the changes in an abandoned pull request are no longer needed (e.g. due to refactoring of the code base or a design change), *assignee* will simply close the pull request.
214
227
215
228
## Making Breaking Changes
216
229
@@ -235,7 +248,7 @@ we encourage contributors to follow these common engineering practices:
235
248
236
249
* Format commit messages following these guidelines:
237
250
238
-
```
251
+
```text
239
252
Summarize change in 50 characters or less
240
253
241
254
Similar to email, this is the body of the commit message,
@@ -308,3 +321,6 @@ Once you sign a CLA, all your existing and future pull requests will be labeled
Copy file name to clipboardExpand all lines: .github/PULL_REQUEST_TEMPLATE.md
+1-1Lines changed: 1 addition & 1 deletion
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -3,6 +3,6 @@
3
3
If you are a PowerShell Team member, please make sure you choose the Reviewer(s) and Assignee for your PR.
4
4
If you are not from the PowerShell Team, you can leave the fields blank and the Maintainers will choose them for you. If you are familiar with the team, feel free to mention some Reviewers yourself.
5
5
6
-
For more information about the roles of Reviewer and Assignee, refer to CONTRIBUTING.md.
6
+
For more information about the roles of Reviewer and Assignee, refer to [CONTRIBUTING.md](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md).
0 commit comments