GeezerVison

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

The link should remove one #area-experts

Will fix

same to the '6.'

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.

will fix

same here to these 2 sub items.

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

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'?

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 Dongbo Wang (@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.

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

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

are any anyone

Remove the any

will fix

functional, performance, or reliability

Add security.

will fix

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.

agree

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"

yes

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

I agree with Dongbo Wang (@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?

Ilya (@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 Travis Plunk (@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 Ilya (@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.

Dongbo Wang (@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.

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