Update coding guidelines #4754

daxian-dbw · 2017-09-05T19:25:19Z

Update coding guidelines to make it more concrete and useful in a review process.

daxian-dbw · 2017-09-05T19:58:17Z

Please feel free to question the content I put in the doc as well as suggest more content.

andyleejordan · 2017-09-05T20:02:15Z

.github/CONTRIBUTING.md

-  A better example is: "Add Ensure parameter to New-Item cmdlet", with "Fixes #5" in the PR's body.
+  Don't simply put: "Fix issue #5".
+  Also don't directly use the issue title as the PR title.
+  An issue title is to briefly describe what is wrong, while a PR title is to briefly describe what is changed.


I like this.

May be worth adding that the body of the PR is used to create the ChangeLog, so try to have the first sentence explain benefit to end user

Added it to the doc.

andyleejordan · 2017-09-05T20:03:52Z

docs/dev-process/coding-guidelines.md

+
+* Fields should be specified at the top within type declarations.
+
+* File encoding should be `ASCII` (preferred) or `UTF8` (with `BOM`) if absolutely necessary.


Maybe a note to explicitly avoid UTF-16, and a warning that some Windows editors may default to this.

I'd like to disallow all BOM encodings - tests that need a BOM are better off generating the file anyway.

Fixed -- Mention that all bom encoding should be avoided.

andyleejordan · 2017-09-05T20:06:21Z

docs/dev-process/coding-guidelines.md

-All new features should support CoreCLR.
-
-## Performance considerations
+## Performance Considerations


Maybe a note about avoiding exceptions as much as possible? Especially avoid using exceptions for logic (reserve them for unrecoverable errors such as those from the OS or libraries).

Good point. Added.

andyleejordan · 2017-09-05T20:07:19Z

docs/dev-process/coding-guidelines.md

+  but not required.
+
+* Stick to the `DRY` principle -- Don't Repeat Yourself.
+   * Wrap the commonly used code in methods, or even put it in a utility class if that makes sense,


Maybe mention some of the existing utility classes.

Add an example utility class System.Management.Automation.StringToBase64Converter

andyleejordan

Left some ideas, but looks great overall.

SteveL-MSFT · 2017-09-05T20:54:25Z

.github/CONTRIBUTING.md

-  A better example is: "Add Ensure parameter to New-Item cmdlet", with "Fixes #5" in the PR's body.
+  Don't simply put: "Fix issue #5".
+  Also don't directly use the issue title as the PR title.
+  An issue title is to briefly describe what is wrong, while a PR title is to briefly describe what is changed.


May be worth adding that the body of the PR is used to create the ChangeLog, so try to have the first sentence explain benefit to end user

SteveL-MSFT · 2017-09-05T20:55:56Z

docs/dev-process/coding-guidelines.md

+* Add comments when a reviewer needs help to understand your changes.

-### Runtimes
+* Update existing comments when you are changing the corresponding code.


Update/remove

PaulHigin · 2017-09-05T23:20:47Z

docs/dev-process/coding-guidelines.md

+
+## Security Considerations
+
+Security is an important aspect of PowerShell and we need to be very careful about changes that may introduce security risks.


I feel we should say a little more about what security risks are and should at least mention input validation, code injection, privilege escalation, data privacy, and DOS concerns. We should have a separate document that goes into more detail but that can be a separate effort.

input validation - this could be easily confused with the input validation within an API. Could you please elaborate a bit on it in the security context?

data privacy - do you mean data privacy breach? Can you please give an example?

We should have a separate document that goes into more detail

That would be very useful. Can you please work on such a doc? If you are busy on things and cannot get to it in near future, I'm happy to work on a draft if you can provide me some resources and pointers.

Has CoreFX such document?

Input validation for public API or Cmdlet is exactly what I mean :). That is where code injection can occur and I feel being aware of it should be part of all code reviews. Likewise data privacy is being aware that private data must always be protected. It is not something we have to be too concerned with with PowerShell (other than credentials and transferring remote data) but again something to be aware of during a code review.

I think we should just mention basic security areas to be aware of during a code review in a general way here, and then provide a link to a separate document where we can provide more details and examples. I am happy to work on such a document but I am not sure when I can get to it. I'll create an issue to track the work.

Add the following:

... may introduce security risks, such as code injection caused by the lack of input validation, privilege escalation due to the misuse of impersonation, or data privacy breach with a plain text password.

iSazonov

Sorry for many comments - most is minor - feel free to ignore.

iSazonov · 2017-09-06T05:14:29Z

docs/dev-process/coding-guidelines.md

+
+When making changes, you may find some existing code goes against the conventions defined here.
+In such cases, please avoid reformatting any existing code when submitting a PR as it obscures the functional changes of the PR.
+A separate PR should be submitted for style-only changes.


What is the preferred process?

Block current PR1

Open style PR and merge

Rebase and continue PR1

or

Open new tracking Issue

Continue and merge current PR

Open new PR to fix the tracking Issue - Should we address this to the author of the first PR?

Here are my experiences:

Depending on how bad the reformatting affects the review, I will choose to either tolerate the formatting changes or block the PR and ask the author to revert the formatting changes.

For example, some PRs has changes that remove the trailing spaces. When that kind of formatting changes doesn't affect review much, I will tolerate them.

But for some other PRs, the formatting changes might cause the review to be difficult, and I will block the PR and ask the author to revert formatting changes.

We ask authors to revert the formatting changes and suggest them to submit a separate PR, but they are not required to submit a PR to fix the formatting. You can give them the choices as for how to revert the reformatting changes -- submit style PR and merge and then rebase the original PR is certainly one way to do it; or they can just revert the formatting changes and submit a style PR later.

I don't like tracking issue in this situation.

I unlike tracking issue too. But without it we can forget and lost. So we should open style PR before continue original PR.

I don't think we should make it a requirement for the author to submit a style-only PR in this situation. It's acceptable if they just revert the formatting changes, and it would be a plus if they are willing to go one step further to fix the style/formatting in a separate PR as well.

iSazonov · 2017-09-06T05:16:52Z

.github/CONTRIBUTING.md


 ![Github-PR-dev.png](Images/Github-PR-dev.png)

+* It's recommended that the lines of changes in a PR should not be too big,


"It's recommended to keep PR as small as possible"

Words adjusted.

iSazonov · 2017-09-06T05:22:55Z

docs/dev-process/coding-guidelines.md

+
+* Avoid more than one blank empty line at any time.
+
+* Avoid unnecessary trailing spaces at the end of a line.


Where we have "necessary" trailing spaces at the end of a line?
It seems we recommend to configure VS Code to automatically remove trailing spaces.

Will remove unnecessary.

iSazonov · 2017-09-06T05:26:45Z

docs/dev-process/coding-guidelines.md

+
+* Use four spaces of indentation (no tabs).
+
+* Avoid more than one blank empty line at any time.


Maybe add "Split logical code blocks by one blank empty line. "
Maybe add "Split code regions by three blank empty lines." Ex.: some files define some classes.

The layout conventions are mostly from https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md

I think we shouldn't go that details about how to use empty lines.

iSazonov · 2017-09-06T05:32:41Z

docs/dev-process/coding-guidelines.md

-  use `readonly` where possible.
+
+* Namespace imports should be specified at the top of the file,
+  outside of `namespace` declarations and should be sorted alphabetically.


The follow pattern looks fine and we use it:

Put on first place using System.* sorted alphabetically

Then put blank line and separate block third party namespaces sorted alphabetically.

Yes, sorted alphabetically is too much detailed. I will remove it.

iSazonov · 2017-09-06T06:32:51Z

docs/dev-process/coding-guidelines.md

+      or even put it in a utility class if that makes sense,
+      so that the same code can be reused (i.e. `StringToBase64Converter.Base64ToString(string)`).
+    * Check if the code for the same purpose already exists in the code base before inventing your own wheel.
+    * Avoid repeating literal strings in code. Instead, use `const` variable to hold the string.


Put strings that are displayed to users in resx files that can be localized later.

Will add it.

iSazonov · 2017-09-06T06:43:49Z

docs/dev-process/coding-guidelines.md

+  without causing performance concerns in performance-sensitive code.

+* We produce a single binary for all UNIX variants,
+  so runtime checks are currently necessary for some of them (e.g. macOS vs. Linux).


Do we require the directives to be placed at the beginning of the line?
(VS Code expand/collapse right the directives only if they indent with surrounding code - should we consider this as bug or we can indent the directives?)

Will mention this in this section.

Mentioned in ### Layout Conventions as it's related to the code layout.

iSazonov · 2017-09-06T06:56:12Z

docs/dev-process/coding-guidelines.md

+Currently, @PaulHigin and @TravisEz13 are our security SMEs.
+See [CODEOWNERS](../../.github/CODEOWNERS) for more information about the area experts.
+
+## Best Practices


Maybe add that we should follow CoreFX everywhere:

use CoreFX APIs

request CoreFX to enhance API when we need the new API

fix CoreFX bugs in CoreFX repo

I don't think that belongs to coding best practices. And use CoreFX APIs is mentioned in the portable code section:

Going forward, we try to depend on .NET Core to handle platform differences, so avoid adding new P/Invoke calls where a suitable alternative exists in .NET Core.

iSazonov · 2017-09-06T07:04:53Z

docs/dev-process/coding-guidelines.md

-* Avoid LINQ - it can create lots of avoidable garbage
+* Avoid LINQ - it can create lots of avoidable garbage.
+
 * Prefer `for` and `foreach`,


Prefer over LINQ?

What do you mean here?

What is a context here? "We prefer use 'for' instead of LINQ"?

Yes, I think it's prefer looping over collections directly over LINQ.
I will try to clarify.

iSazonov · 2017-09-06T07:15:21Z

docs/dev-process/coding-guidelines.md

+  Instead, reuse the static ones via `Utils.EmptyArray<T>`.
+
 * Avoid unnecessary memory allocation in a loop.
  Move the memory allocation outside the loop if possible.


I remember a recommendation to use static RegEx to benefit caching. Is this relevant? Maybe still mention compile RegEx and using StringBuilder if we have more 3-4 re-allocations.

I think StringBuilder is worth mentioning -- string operations are quite common. RegEx is too specific to me.

vors · 2017-09-06T07:10:37Z

docs/dev-process/coding-guidelines.md

+### Naming Conventions
+
+* Use meaningful, descriptive words for names.
+  For method names, it's encouraged to use `Verb-Object` pair such as **`LoadModule`**.


Verb-Object pair such as LoadModule.

shoud it be VerbObject then?

vors · 2017-09-06T07:11:41Z

docs/dev-process/coding-guidelines.md

+
+* Use `_camelCase` to name internal and private fields and use `readonly` where possible.
+  Prefix instance fields with `_`, static fields with `s_` and thread static fields with `t_`.
+  When used on static fields, `readonly` should come after `static` (i.e. `static readonly` not `readonly static`).


this is generally a linter job to enforce such things.
We have a hand-written linter for tabs vs spaces and such.
Do we consider to add a proper dotnet linter to CI and build.psm1?

@TravisEz13 has some ideas to enforce the formatting check in CI.

vors · 2017-09-06T07:20:24Z

docs/dev-process/coding-guidelines.md


-Any other preprocessor defines found in the source are used for one-off custom builds,
-typically to help debug specific scenarios.
+* Add comments when a reviewer needs help to understand your changes.


I think phrasing can be better here. It may be perceived as encouragement for creating a historical log in the comments, i.e.

// This line used to be bla-bla // and was changed to bla-bla because previously we had a problem in bla-bla corner case.

The sentences are reworded to:

* Add comments where the code is not trivial or could be confusing. * Add comments where a reviewer needs help to understand the code.

daxian-dbw · 2017-09-06T19:21:27Z

All comments are addressed. Please take another look, thanks!

iSazonov

Thanks!

daxian-dbw · 2017-09-07T15:27:56Z

@PaulHigin your comment has been addressed. Can you please take another look?

PaulHigin

LGTM

iSazonov · 2017-09-08T10:07:41Z

@daxian-dbw Is the PR ready to merge? (Squash and merge?)

daxian-dbw · 2017-09-08T16:05:49Z

@iSazonov Yes, I think it's ready to go.

iSazonov · 2017-09-08T16:24:51Z

@daxian-dbw Many thanks!

Update coding guidelines

be5acb2

daxian-dbw assigned iSazonov Sep 5, 2017

daxian-dbw requested review from PaulHigin, SteveL-MSFT, TravisEz13, andyleejordan, iSazonov, lzybkr, mirichmo and vors September 5, 2017 19:25

msftclas added the cla-not-required label Sep 5, 2017

Add a section about security

bf82a78

andyleejordan reviewed Sep 5, 2017

View reviewed changes

andyleejordan approved these changes Sep 5, 2017

View reviewed changes

lzybkr approved these changes Sep 5, 2017

View reviewed changes

SteveL-MSFT reviewed Sep 5, 2017

View reviewed changes

daxian-dbw added 2 commits September 5, 2017 15:23

Address comments

ed3c66f

Update .spelling

13f680a

SteveL-MSFT approved these changes Sep 5, 2017

View reviewed changes

PaulHigin suggested changes Sep 5, 2017

View reviewed changes

iSazonov reviewed Sep 6, 2017

View reviewed changes

vors reviewed Sep 6, 2017

View reviewed changes

daxian-dbw added 4 commits September 6, 2017 11:23

Address more comments

6353286

Address comment about the security section

f42c55d

Fix 'i.e.' and 'e.g.'

276255d

Update .spelling

5479132

One more tweak

f5b528f

iSazonov approved these changes Sep 7, 2017

View reviewed changes

TravisEz13 approved these changes Sep 7, 2017

View reviewed changes

PaulHigin approved these changes Sep 7, 2017

View reviewed changes

Add motivation for using 'nameof'

a69cf4d

iSazonov merged commit ca1d2ac into PowerShell:master Sep 8, 2017

daxian-dbw deleted the docs branch September 8, 2017 17:43


		* Fields should be specified at the top within type declarations.

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


		## Security Considerations

		Security is an important aspect of PowerShell and we need to be very careful about changes that may introduce security risks.


		![Github-PR-dev.png](Images/Github-PR-dev.png)

		* It's recommended that the lines of changes in a PR should not be too big,


		* Avoid more than one blank empty line at any time.

		* Avoid unnecessary trailing spaces at the end of a line.


		* Use four spaces of indentation (no tabs).

		* Avoid more than one blank empty line at any time.

Update coding guidelines #4754

Update coding guidelines #4754

Uh oh!

Conversation

daxian-dbw commented Sep 5, 2017

Uh oh!

daxian-dbw commented Sep 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyleejordan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Sep 6, 2017 •

edited

Loading