update of pull-request-process #4710

SteveL-MSFT · 2017-08-30T21:38:41Z

update pull-request-process to clarify role/responsibility of author and reviewers based on recent feedback from team and community. Also moved it to .github/CONTRIBUTING.md so that all related information can be found at one place.

…nd reviewers

daxian-dbw · 2017-08-30T23:32:49Z

docs/dev-process/pull-request-process.md

+1. The *author* ensures that their pull request passes the [CI system][ci-system] build.
+  - If the build fails, a [Repository Maintainer][repository-maintainer] adds the `Review - waiting on author` label to the pull request. 
+  The *author* can then continue to update the pull request until the build passes.
+3. If the *author* knows whom should participate in the review, they should add them otherwise they can add the recommended *reviewers*.


this should be 1. as well to keep consistent with others.

I had it as '1', but it doesn't get rendered correctly (at least in VSCode)

It should work in Github, see https://github.com/daxian-dbw/PowerShell/blob/temp/docs/dev-process/pull-request-process.md

My quick change is at daxian-dbw@757449e. All I did was to change the number to 1. and make sure there are 3 spaces before every sub-level -.

Maybe remove "recommended reviewers" and say about "code owners" - that we use today.

Ok, just an issue with vscode markdown rendering, I'll change all these

daxian-dbw · 2017-08-30T23:33:30Z

docs/dev-process/pull-request-process.md

+  - 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), simply close the pull request.
+
+[repository-maintainer]: ../community/governance.md#repository-maintainers
+[area-expert]: ../community/governance.md#area-experts#area-experts


The link should remove one #area-experts

daxian-dbw · 2017-08-30T23:33:43Z

docs/dev-process/pull-request-process.md

+1. An [Area Expert][area-expert] should also review the pull request.
+  - If the *author* does not meet the *reviewer*'s standards, the *reviewer* makes comments. A *maintainer* then removes the `Review - needed` label and adds the `Review - waiting on author` label. The *author* must address the comments and repeat from step 2.
+  - If the *author* meets the *reviewer*'s standards, the *reviewer* approves the PR. A maintainer then removes the `need review` label.
+6. Once the code review is completed, a *maintainer* merges the pull request after one business day to allow for additional critical feedback.


same to the '6.'

daxian-dbw · 2017-08-30T23:35:45Z

docs/dev-process/pull-request-process.md

+
+1. The PR *author* creates a pull request from a fork.
+1. The *author* ensures that their pull request passes the [CI system][ci-system] build.
+  - If the build fails, a [Repository Maintainer][repository-maintainer] adds the `Review - waiting on author` label to the pull request. 


there should be 3 spaces before - so that it could be indented correctly.
This comment applies to all instances where sub bullets exist in a list entry.
I know some of these problems have been there before this change, but it's better to fix them to make the doc more readable.

Currently we don't use the labels. 😕

Yes, strictly applying correct labels means the maintainer needs to closely monitor the status changes of a PR, e.g. has any reviewer finished reviewing and waiting on the author to address comments? has the author addressed comments and ready to have reviewers take another look? was the CI failed? I think that's not a good use of the maintainer's time.

All of those status changes are obvious to all parties. Since author should drive the PR, generally the author should ping the reviewers once comments are addressed.

daxian-dbw · 2017-08-30T23:36:01Z

docs/dev-process/pull-request-process.md

+1. Once the build passes, if there is not sufficient review, the *maintainer* adds the `Review - needed` label.
+1. An [Area Expert][area-expert] should also review the pull request.
+  - If the *author* does not meet the *reviewer*'s standards, the *reviewer* makes comments. A *maintainer* then removes the `Review - needed` label and adds the `Review - waiting on author` label. The *author* must address the comments and repeat from step 2.
+  - If the *author* meets the *reviewer*'s standards, the *reviewer* approves the PR. A maintainer then removes the `need review` label.


same here to these 2 sub items.

daxian-dbw · 2017-08-30T23:46:16Z

docs/dev-process/pull-request-process.md

@@ -0,0 +1,51 @@
+# Pull Request Process


There is a duplicate section of the same content in https://github.com/PowerShell/PowerShell/blob/master/docs/maintainers/README.md#pull-request-workflow
We should remove it from the README.md.

I'm going to move this content out of README.md and get rid of this doc and move everything into CONTRIBUTING.md

daxian-dbw · 2017-08-30T23:49:50Z

docs/dev-process/pull-request-process.md

+  - If the *author* meets the *reviewer*'s standards, the *reviewer* approves the PR. A maintainer then removes the `need review` label.
+6. Once the code review is completed, a *maintainer* merges the pull request after one business day to allow for additional critical feedback.
+
+### Roles and Responsibilities


There is a similar section in https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---code-review. For example, both mention 'author should drive the PR'. Maybe we should somehow link these two, or maybe we move this to the 'CONTRIBUTING.md'?

daxian-dbw · 2017-08-31T00:00:25Z

docs/dev-process/pull-request-process.md

+  - `Comment` if you are making suggestions that the *author* does not have to accept.
+  Early in the review, it is acceptable to provide feedback on coding style based on the published [Coding Guidelines](coding-guidelines), however, after
+  the *author* has already pushed commits to address feedback, it is generally _not_ acceptable to focus on style issues as it disrupts the PR process.
+  Late feedback can be submitted as a new issue or new pull request from the *reviewer*.


I think this depends on whether the late feedback is critical.
If maintainers can enforce the 1-day-action policy, then no comment is a late comment. Do you mean to refer to 'late comment' as comments made after approval(s)?

Yes, I'll clarify that.

I agree with @daxian-dbw. What is "style issues"? If the commits contains bad code, bad patterns and broken formattings we should request obligatory changes in any time. Any policy that allow pass bad PR is bad policy.
Currently we fast merge with ignoring minor issues only "blocking" and "critical" PRs. Also I think we can fast merge if author ask us (busy, vacation...) but before that, we have to open the tracking Issue and determine (PR Assignee) exactly who will do fix it as soon as possible. In fact, we are doing so.

daxian-dbw · 2017-08-31T00:01:30Z

docs/dev-process/pull-request-process.md

+  Early in the review, it is acceptable to provide feedback on coding style based on the published [Coding Guidelines](coding-guidelines), however, after
+  the *author* has already pushed commits to address feedback, it is generally _not_ acceptable to focus on style issues as it disrupts the PR process.
+  Late feedback can be submitted as a new issue or new pull request from the *reviewer*.
+3. *Maintainers* ensure that proper review has occurred and if they believe one approval is not sufficient, the *maintainer* is responsible to add more reviewers.


this should be '1.', to keep consistency.

removed redundant info from maintainters README.md remove pull-request-process.md

daxian-dbw · 2017-08-31T17:15:43Z

.github/CONTRIBUTING.md

+   When updating your pull request, please **create new commits** and **don't rewrite the commits history**. 
+   This way it's very easy for the reviewers to see diff between iterations.
+   If you rewrite the history in the pull request, review could be much slower.
+   Once the review is done, you can rewrite the history to make it prettier, if you like.


Let's not ask authors to rewrite history after approval. The author might add codes that was not there during the review, and it would be impossible for us to know.

I'll remove this, it was there previously, but I agree it doesn't make sense

daxian-dbw · 2017-08-31T17:16:11Z

.github/CONTRIBUTING.md

+   If you rewrite the history in the pull request, review could be much slower.
+   Once the review is done, you can rewrite the history to make it prettier, if you like.
+   Otherwise it's likely would be squashed on merge to master by the *assignee*.
+1. *Reviewers* are any anyone who wants to contribute.


are any anyone

Remove the any

daxian-dbw · 2017-08-31T17:17:00Z

.github/CONTRIBUTING.md

+   Once the review is done, you can rewrite the history to make it prettier, if you like.
+   Otherwise it's likely would be squashed on merge to master by the *assignee*.
+1. *Reviewers* are any anyone who wants to contribute.
+   They are responsible for ensuring the code: addresses the issue being fixed, does not create new issues (functional, performance, or reliability), and implements proper design.


functional, performance, or reliability

Add security.

daxian-dbw · 2017-08-31T17:23:25Z

.github/CONTRIBUTING.md

+   - `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.
+   - `Comment` if you are making suggestions that the *author* does not have to accept.
+   Early in the review, it is acceptable to provide feedback on coding style based on the published [Coding Guidelines](../docs/dev-process/coding-guidelines.md), however, after
+   the *author* has already pushed commits to address feedback, it is generally _not_ acceptable to focus on style issues as it disrupts the PR process.


after the author has already pushed commits to address feedback

I think it should be after the PR has been approved, because before a PR is approved, there might be multiple iterations and during that iterations it may be OK to bring up such comments -- as long as they are legit based on our code style guideline.

daxian-dbw · 2017-08-31T17:25:58Z

.github/CONTRIBUTING.md

+   Non-critical late feedback (after PR has been approved) can be submitted as a new issue or new pull request from the *reviewer*.
+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.
+   An *assignee* may also be a reviewer, but the roles are distinct.
+   Once the PR has been approved and the CI system is passing, the *assignee* will merge the PR.


the assignee will merge the PR.

Should be merge the PR after one business day. We mentioned the one business day rule above in "Pull Request - Workflow"

daxian-dbw

LGTM

iSazonov · 2017-08-31T18:47:05Z

.github/CONTRIBUTING.md

+   - `Comment` if you are making suggestions that the *author* does not have to accept.
+   Early in the review, it is acceptable to provide feedback on coding style based on the published [Coding Guidelines](../docs/dev-process/coding-guidelines.md), however,
+   after the PR has been approved, it is generally _not_ acceptable to focus on style issues as it disrupts the PR process.
+   Non-critical late feedback (after PR has been approved) can be submitted as a new issue or new pull request from the *reviewer*.


I repeat my comment (it seems it may be lost):

I agree with @daxian-dbw. What is "style issues"? If the commits contains bad code, bad patterns and broken formattings we should request obligatory changes in any time. Any policy that allow pass bad PR is bad policy.
Currently we fast merge with ignoring minor issues only "blocking" and "critical" PRs. Also I think we can fast merge if author ask us (busy, vacation...) but before that, we have to open the tracking Issue and determine (PR Assignee) exactly who will do fix it as soon as possible. In fact, we are doing so.

The intent here (if it's not captured well, let me know how to fix) is that we welcome style issues along with other types of issues, but if the only remaining problems after a PR has been approved, it's generally not acceptable to provide only style issues. In this situation, if the issue is deemed blocking, then go ahead and submit the feedback. If not and a maintainer, go ahead and make the changes directly. If not a maintainer, open a new code cleanup issue or submit a follow-up PR.

I am 11 months in the project and trying look every PR - I never see any problems with style change requests - all authors always address such requests quickly. Moreover we have many PR in which last commits is just fix style issues - extra lines, spaces, unneeded and obvious comments, commented code and so on. It's a really minute job, but we're getting a clean code.
If we're going to skip the dirty code who's going to peel it? Assignee who pass it?

@iSazonov I appreciate all the work and impact you've provided, so try not to take this one personally as although this update coincides with a recent PR, I've received similar feedback from others. I'm hoping that your metatests along with potentially some other tooling suggestions @TravisEz13 has made internally, we don't have to worry as much about style and focus on the code.

A bigger part of this doc change is getting people to use the GitHub comment system to consistently indicate whether they are done with the review, and if the feedback is blocking or not.

For the first two months of this project, I was very amazed at the demands of maintainers for strict style. Later, when I had the experience of writing code for the project, I realized that it was not only justified, but necessary - following a good style saves our time and protects us from making mistakes.
In the project we can see samples of good style code and bad style code. I'm sure everyone would prefer to work with good style code to keep their strength and their time, but at the same time some people you say don't want to create good style code. I don't understand why we should replace good policy with bad policy. I believe MSFT should provide the best practices, especially in public projects, that are good for her reputation.

And for the word code style issue, maybe it's too general because I feel what @iSazonov has in mind are issues like repeated codes (violating the DRY rule) and other things that actually are wrong coding patterns. For those, we definitely want the feedback to be addressed.

Maybe we should be more specific and use the word code formating issue?
Below are the formatting issue examples listed in 'Coding Guidelines', and I believe they are more in line with what we mean here.

A basic rule of formatting is to use "Visual Studio defaults".
Here are some general guidelines

No tabs, indent 4 spaces.

Braces usually go on their own line, with the exception of single line statements that are properly indented.

Use _camelCase for instance fields, use readonly where possible.

Use of this is neither encouraged nor discouraged.

Avoid more than one blank empty line.

Public members should use doc comments, internal members may use doc comments but it is not encouraged.

Public members in a namespace that ends with Internal, for example System.Management.Automation.Internal are not considered a supported public API. Such members are necessarily public as implementation details in code shared between C# and PowerShell script, or must be available publicly by generated code.

File encoding should be ASCII (preferred) or UTF8 (with BOM) if absolutely necessary.

BTW, I will submit a PR soon to add additional things to the "Coding Guidelines" doc. We can together make that doc a better reference to use during code review time.

For me style == formatting only, but I can see how it might be interpreted differently. I'm fine changing it to formatting since that's what we use in the coding guidelines. Agree that non-formatting feedback should always be given although even in those cases, some can be taken as a different issue.

I see different understanding the term "style" too.

@daxian-dbw It is good idea to enhance the "Coding Guidelines". We should think about sync our Meta tests with "Coding Guidelines" doc and how activate the tests.

I absolutely agree.

… Coding Guidelines

daxian-dbw

LGTM

TravisEz13 · 2017-09-01T18:57:21Z

.github/CONTRIBUTING.md

@@ -167,50 +167,55 @@ Additional references:
  [run the spellchecker command line tool in interactive mode](#spellchecking-documentation)
  to add words to the `.spelling` file.

-#### Pull Request - Code Review


Suggestion: Can you add this file to https://github.com/PowerShell/PowerShell/blob/master/test/common/markdown/markdown.tests.ps1#L74 to make sure your changes (and the rest of the file.) follow the correct and consistent syntax.

I'll make that change

TravisEz13

no blocking issues

update pull-request-process to clarify role/responsiblity of author a…

0cda0d9

…nd reviewers

SteveL-MSFT assigned daxian-dbw Aug 30, 2017

SteveL-MSFT requested review from daxian-dbw, joeyaiello and lzybkr August 30, 2017 21:38

msftclas added the cla-not-required label Aug 30, 2017

daxian-dbw requested changes Aug 31, 2017

View reviewed changes

daxian-dbw requested a review from TravisEz13 August 31, 2017 00:02

merge pull request content into CONTRIBUTING.md

c1c6cce

removed redundant info from maintainters README.md remove pull-request-process.md

SteveL-MSFT force-pushed the pull-request-process branch from 14fd245 to c1c6cce Compare August 31, 2017 16:52

daxian-dbw requested changes Aug 31, 2017

View reviewed changes

address PR feedback

83e10ab

daxian-dbw approved these changes Aug 31, 2017

View reviewed changes

iSazonov reviewed Aug 31, 2017

View reviewed changes

SteveL-MSFT mentioned this pull request Aug 31, 2017

Added support for converting enums as strings in JSON #4318

Merged

SteveL-MSFT added 2 commits August 31, 2017 14:59

softened languaged on style feedback after approval

bf67597

changed word style to formatting to conform to language we use in…

ae7340d

… Coding Guidelines

daxian-dbw approved these changes Aug 31, 2017

View reviewed changes

TravisEz13 reviewed Sep 1, 2017

View reviewed changes

TravisEz13 approved these changes Sep 1, 2017

View reviewed changes

SteveL-MSFT added 2 commits September 1, 2017 14:54

added CONTRIBUTING.md to markdown tests

21f0b96

fixed markdown issues

0f00dc8

SteveL-MSFT force-pushed the pull-request-process branch from c4b7485 to 0f00dc8 Compare September 1, 2017 23:11

daxian-dbw merged commit efa320c into PowerShell:master Sep 4, 2017

SteveL-MSFT deleted the pull-request-process branch September 4, 2017 21:03

update of pull-request-process #4710

update of pull-request-process #4710

Conversation

SteveL-MSFT commented Aug 30, 2017 • edited by daxian-dbw Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

daxian-dbw Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TravisEz13 left a comment

Choose a reason for hiding this comment

SteveL-MSFT commented Aug 30, 2017 •

edited by daxian-dbw

Loading

daxian-dbw Aug 31, 2017 •

edited

Loading

daxian-dbw Aug 31, 2017 •

edited

Loading

daxian-dbw Aug 31, 2017 •

edited

Loading