Fix some style issues in engine code by iSazonov · Pull Request #7246 · PowerShell/PowerShell

Ilya (iSazonov) · 2018-07-09T06:27:40Z

PR Summary

The PR resolves some style issues in engine code. (Moved from PR #7242. )

PR Checklist

This change is

Bruce Payette (BrucePay)

There are a couple of things I think you should fix.

Bruce Payette (BrucePay) · 2018-07-09T17:46:01Z

        /// <summary>
        /// The FormatData specified with constructor. This can be null if
-        /// FileName or FormatTable constructor is used
+        /// FileName or FormatTable constructor is used.


Probably should be: "This can be null if the FileName or FormatTable constructors are used"

Bruce Payette (BrucePay) · 2018-07-09T17:48:21Z

-        ///  Define an alias entry to add to the initial session state
+        ///  Define an alias entry to add to the initial session state.
        /// </summary>
        /// <param name="name">Name of the alias</param>


Should be "The name of the alias entry to add."

Bruce Payette (BrucePay) · 2018-07-09T17:48:55Z

-        ///  Define an alias entry to add to the initial session state
+        ///  Define an alias entry to add to the initial session state.
        /// </summary>
        /// <param name="name">Name of the alias</param>


Should be "The name of the alias entry to add."

Bruce Payette (BrucePay) · 2018-07-09T17:49:46Z

-        ///  Define an alias entry to add to the initial session state
+        ///  Define an alias entry to add to the initial session state.
        /// </summary>
        /// <param name="name">Name of the alias</param>


Should be "The name of the alias entry to add."

Bruce Payette (BrucePay) · 2018-07-09T17:51:00Z

+        /// Clone this collection.
        /// </summary>
-        /// <returns>The cloned object</returns>
+        /// <returns>The cloned object.</returns>


"The cloned collection."

Bruce Payette (BrucePay) · 2018-07-09T17:55:40Z


        /// <summary>
-        /// Need to have SnapIn support till we move to modules
+        /// Need to have SnapIn support till we move to modules.


We don't support snapins in PSCore so maybe we should be removing these entries?

The PR is only style issues - please open new tracking Issue.

Bruce Payette (BrucePay) · 2018-07-09T18:01:15Z

-                                    false))
+                                if (SessionStateUtilities.MatchesAnyWildcardPattern(element.Name,
+                                                                                    exportedVariables,
+                                                                                    defaultValue: false))


I would suggest lining up all three parameters but with less indentation.

if (SessionStateUtilities.MatchesAnyWildcardPattern( element.Name, exportedVariables, defaultValue: false))

Bruce Payette (BrucePay) · 2018-07-09T18:02:05Z

+        /// <param name="moduleManifestPath">The manifest that generated the hashtable.</param>
+        /// <param name="key">the table key to use.</param>
+        /// <param name="manifestProcessingFlags">Specifies how to treat errors and whether to load elements.am>
+        /// <param name="result">Value from the manifest converted to the right type.</param>


What's the "am" here?

Bruce Payette (BrucePay) · 2018-07-09T18:03:27Z

-        /// <param name="module">module to remove</param>
-        ///  <param name="moduleNameInRemoveModuleCmdlet">module name specified in the cmdlet</param>
+        /// <param name="module">module to remove.</param>
+        ///  <param name="moduleNameInRemoveModuleCmdlet">module name specified in the cmdlet.</param>


Extra space before "<param" ?

Bruce Payette (BrucePay) · 2018-07-09T18:06:21Z

@@ -1053,9 +1059,14 @@ private static void SortAndRemoveDuplicates<T>(List<T> input, Func<T, string> ke
        /// <param name="variablePatterns">Patterns describing the variables to export</param>
        /// <param name="doNotExportCmdlets">List of Cmdlets that will not be exported,
        ///     even if they match in cmdletPatterns.</param>


Extra spaces before 'even'

Ilya (iSazonov) · 2018-07-10T03:55:17Z

Bruce Payette (@BrucePay) Your feedback was addressed.

Ilya (iSazonov) · 2018-07-17T05:49:46Z

Bruce Payette (@BrucePay) Please update your review.

Travis Plunk (TravisEz13) · 2018-07-18T17:54:30Z

Bruce Payette (@BrucePay) Can you update your review?

Ilya (iSazonov) · 2018-08-09T04:48:15Z

Travis Plunk (@TravisEz13) Bruce Payette (@BrucePay) Dongbo Wang (@daxian-dbw) Could you please review?

Dongbo Wang (daxian-dbw)

Left a few comments

Dongbo Wang (daxian-dbw) · 2018-08-09T21:38:04Z

-        //If used on command collection entry, then for the same name, one can have multiple output
+        // To find the entries based on name.
+        // Why collection - Different SnapIn/modules and same entity names.
+        // If used on command collection entry, then for the same name, one can have multiple output


These comments are duplicate with the ones below in
. They can be removed.

Dongbo Wang (daxian-dbw) · 2018-08-09T21:39:25Z

-        //This one is for module. Can take only one module, and not collection
-        //public static InitialSessionState Create(Module);
+        // This one is for module. Can take only one module, and not collection
+        // public static InitialSessionState Create(Module);


Seems to be these two can be removed.

Dongbo Wang (daxian-dbw) · 2018-08-09T21:43:57Z

+        /// <param name="data">The hashtable to look for the key in.</param>
+        /// <param name="moduleManifestPath">The manifest that generated the hashtable.</param>
+        /// <param name="key">the table key to use.</param>
+        /// <param name="manifestProcessingFlags">Specifies how to treat errors and whether to load elements.>


</param> is missing

Dongbo Wang (daxian-dbw)

Ilya (@iSazonov) Can you please update the PR title and summary to be more related to the changes?

new commits were pushed to address the original comments.

Ilya (iSazonov) · 2018-08-10T18:08:12Z

Dongbo Wang (@daxian-dbw) Title and summary was updated.

Travis Plunk (TravisEz13)

Reviewed 2 of 12 files at r1, 9 of 11 files at r3, 7 of 11 files at r4, 2 of 2 files at r5.
Reviewable status: complete! all files reviewed

The PR resolves some style issues in engine code. (Moved from PR PowerShell#7242. )

Ilya (iSazonov) added 5 commits July 9, 2018 11:12

Fix style issues 1

0e16e78

Fix style issues 2

17c75d6

Fix style issues 3

aef6ee4

Fix style issues 4

58751b0

[Feature] Fix style issues 5

fc95e17

Ilya (iSazonov) assigned Travis Plunk (TravisEz13) Jul 9, 2018

Ilya (iSazonov) requested review from Travis Plunk (TravisEz13) and Dongbo Wang (daxian-dbw) July 9, 2018 06:27

Ilya (iSazonov) requested a review from Bruce Payette (BrucePay) as a code owner July 9, 2018 06:27

Ilya (iSazonov) mentioned this pull request Jul 9, 2018

Add support to experimental features #7242

Merged

8 of 11 tasks

Bruce Payette (BrucePay) previously requested changes Jul 9, 2018

View reviewed changes

Address feedback

66d6259

Merge branch 'master' into style-fixes-for-exp

5644fba

Merge branch 'master' into style-fixes-for-exp

2803e7f

Dongbo Wang (daxian-dbw) reviewed Aug 9, 2018

View reviewed changes

Fix Dongbo's comments

d7c2883

Dongbo Wang (daxian-dbw) approved these changes Aug 10, 2018

View reviewed changes

Ilya (iSazonov) changed the title ~~Fix some style issues for PR #7242~~ Fix some style issues in engine code Aug 10, 2018

Travis Plunk (TravisEz13) approved these changes Aug 14, 2018

View reviewed changes

Travis Plunk (TravisEz13) merged commit 4ee1973 into PowerShell:master Aug 14, 2018

Ilya (iSazonov) deleted the style-fixes-for-exp branch August 14, 2018 05:31

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026

Fix some style issues in engine code (PowerShell#7246)

d248d6b

The PR resolves some style issues in engine code. (Moved from PR PowerShell#7242. )

Conversation

Ilya (iSazonov) commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Bruce Payette (BrucePay) 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!

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!

Ilya (iSazonov) commented Jul 10, 2018

Uh oh!

Ilya (iSazonov) commented Jul 17, 2018

Uh oh!

Travis Plunk (TravisEz13) commented Jul 18, 2018

Uh oh!

Ilya (iSazonov) commented Aug 9, 2018

Uh oh!

Dongbo Wang (daxian-dbw) 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!

Dongbo Wang (daxian-dbw) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Aug 10, 2018

Uh oh!

Travis Plunk (TravisEz13) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Ilya (iSazonov) commented Jul 9, 2018 •

edited

Loading