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

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

BrucePay

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

BrucePay · 2018-07-09T17:46:01Z

src/System.Management.Automation/engine/InitialSessionState.cs

        /// <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"

BrucePay · 2018-07-09T17:48:21Z

src/System.Management.Automation/engine/InitialSessionState.cs

-        ///  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."

BrucePay · 2018-07-09T17:48:55Z

src/System.Management.Automation/engine/InitialSessionState.cs

-        ///  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."

BrucePay · 2018-07-09T17:49:46Z

src/System.Management.Automation/engine/InitialSessionState.cs

-        ///  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."

BrucePay · 2018-07-09T17:51:00Z

src/System.Management.Automation/engine/InitialSessionState.cs

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


"The cloned collection."

BrucePay · 2018-07-09T17:55:40Z

src/System.Management.Automation/engine/InitialSessionState.cs


        /// <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.

BrucePay · 2018-07-09T18:01:15Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

-                                    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))

BrucePay · 2018-07-09T18:02:05Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

+        /// <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?

BrucePay · 2018-07-09T18:03:27Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

-        /// <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" ?

BrucePay · 2018-07-09T18:06:21Z

src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs

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

iSazonov · 2018-07-10T03:55:17Z

@BrucePay Your feedback was addressed.

iSazonov · 2018-07-17T05:49:46Z

@BrucePay Please update your review.

TravisEz13 · 2018-07-18T17:54:30Z

@BrucePay Can you update your review?

iSazonov · 2018-08-09T04:48:15Z

@TravisEz13 @BrucePay @daxian-dbw Could you please review?

daxian-dbw

Left a few comments

daxian-dbw · 2018-08-09T21:38:04Z

src/System.Management.Automation/engine/InitialSessionState.cs

-        //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.

daxian-dbw · 2018-08-09T21:39:25Z

src/System.Management.Automation/engine/InitialSessionState.cs

-        //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.

daxian-dbw · 2018-08-09T21:43:57Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

+        /// <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

daxian-dbw

@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.

iSazonov · 2018-08-10T18:08:12Z

@daxian-dbw Title and summary was updated.

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

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

iSazonov assigned TravisEz13 Jul 9, 2018

iSazonov requested review from TravisEz13 and daxian-dbw July 9, 2018 06:27

iSazonov requested a review from BrucePay as a code owner July 9, 2018 06:27

iSazonov mentioned this pull request Jul 9, 2018

Add support to experimental features #7242

Merged

11 tasks

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

daxian-dbw reviewed Aug 9, 2018

View reviewed changes

Fix Dongbo's comments

d7c2883

daxian-dbw approved these changes Aug 10, 2018

View reviewed changes

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

TravisEz13 approved these changes Aug 14, 2018

View reviewed changes

TravisEz13 merged commit 4ee1973 into PowerShell:master Aug 14, 2018

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

Conversation

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!

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!

iSazonov commented Jul 10, 2018

Uh oh!

iSazonov commented Jul 17, 2018

Uh oh!

TravisEz13 commented Jul 18, 2018

Uh oh!

iSazonov commented Aug 9, 2018

Uh oh!

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!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Aug 10, 2018

Uh oh!

TravisEz13 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iSazonov commented Jul 9, 2018 •

edited

Loading