Get-Verb: add verb descriptions and alias prefixes by Tadas · Pull Request #4746 · PowerShell/PowerShell

Tadas · 2017-09-03T18:26:49Z

Fixes #4732 - VerbInfo should have a Description member
Fixes #4372 - Get-Verb should list alias prefixes too (extend System.Management.Automation.VerbInfo)

Summary

Added VerbDescriptions and VerbAliasPrefixes static classes with description and alias strings. Not actually sure if this is the best way to store the descriptions.

To keep the descriptions short I only took the first sentence from https://msdn.microsoft.com/en-us/library/ms714428(v=vs.85).aspx "Action" field. Aliases come from the same page

Need document approved prefixes for some new verbs:

iSazonov · 2017-09-04T10:22:44Z

@Tadas Thanks for your contribution!

@lzybkr Could you please comment. Currently we use reflection and dynamically re-create verbinfo objects in Get-Verb cmdlet and also use reflection and build s_validVerbs and s_recommendedAlternateVerbs in Verbs.cs - perhaps the time has come to optimize performance of the code? Can we use someting like we use in Types_Ps1Xml.cs?

lzybkr · 2017-09-04T15:33:14Z

src/System.Management.Automation/utils/Verbs.cs


+    /// <summary>
+    /// Verb descriptions.
+    /// </summary>


Descriptions needs to be in a resx file so they can be localized.

Should it be internal?

lzybkr · 2017-09-04T15:37:48Z

src/System.Management.Automation/utils/Verbs.cs

+
+        /// <summary>
+        /// "Build" verb alias prefix
+        /// </summary>


@HemantMahawar should probably be defining a prefix for the verbs which have none: Build, Deploy, Optimize, and Resize.

Build --> bd

Deploy --> dp

Optimize --> om

Resize --> rz

/cc @joeyaiello

lzybkr · 2017-09-04T15:44:53Z

@Tadas @iSazonov - we don't need a cache for these new properties and reflection is fine - Get-Verb isn't important for performance.

s_recommendedAlternateVerbs should probably be Lazy - it shouldn't be normal to need it.

s_validVerbs could avoid reflection, but use reflection in debug builds to ensure that nothing was missed, but it never showed up in profiles during startup, so the difference might not be measurable.

dantraMSFT · 2017-09-05T18:06:20Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetVerbCommand.cs

                        VerbInfo verb = new VerbInfo();
                        verb.Verb = field.Name;
+
+                        FieldInfo aliasField = typeof(VerbAliasPrefixes).GetField(field.Name);


Suggestion: Refactor this as static method in the associated class. It will provide more flexibility with refactoring, should it be necessary.

dantraMSFT · 2017-09-05T18:07:37Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetVerbCommand.cs

+
                        verb.Group = groupName;
+
+                        FieldInfo descriptionField = typeof(VerbDescriptions).GetField(field.Name);


The description lookup should be a static method in the VerbDescription class; especially since the strings should be in a resx file to enable localization.

iSazonov · 2017-09-08T09:19:37Z

src/System.Management.Automation/utils/Verbs.cs

-        public const string Write = "Adds information to a target";
+        public static string GetVerbDescription(string verb)
+        {
+            return VerbDescriptionStrings.ResourceManager.GetString(verb + "VerbDescription");


I do not see the benefits of "verbdescription". I think we could remove it.

iSazonov · 2017-09-08T09:24:29Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Verb.Tests.ps1

+
+    It "Should have descriptions for all verbs" {
+        $noDesc = Get-Verb | Where-Object { [string]::IsNullOrEmpty($_.Description) }
+        $noDesc | Should be $null


For "all verbs" we should count.

You mean I should test for $verbsWithDescription.count | Should be $allVerbs.count or do you mean something else?

Yes. We should count verbs and verbs with Description.

iSazonov · 2017-09-08T09:25:34Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Verb.Tests.ps1

+
+    It "Should have alias prefixes for all verbs" {
+        $noPrefix = Get-Verb | Where-Object { [string]::IsNullOrEmpty($_.AliasPrefix) }
+        $noPrefix | Should be $null


The same. We should count verbs and verbs with AliasPrefix.

iSazonov · 2017-09-08T09:28:49Z

src/System.Management.Automation/utils/Verbs.cs

+        /// <summary>
+        /// Gets verb prefix
+        /// </summary>
+        public static string GetVerbPrefix(string verb)


Maybe GetVerbAliasPrefix ?

Tadas · 2017-09-08T17:30:11Z

I've just noticed that Copy and Complete verbs have the same alias "cp". Should those be unique?

Tadas · 2017-09-17T13:17:30Z

@HemantMahawar @joeyaiello what do you think of the duplicate verb prefixes for Complete and Copy?

lzybkr · 2017-09-17T23:23:29Z

src/System.Management.Automation/utils/Verbs.cs

+
+    /// <summary>
+    /// Verb Alias prefixes.
+    /// </summary>


Maybe these should be attributes, like:

public static class VerbsCommon { /// <summary> /// Synonyms: Add to, append or attach. /// </summary> [AliasPrefix("a")] public const string Add = "Add"; }

Looks good as common pattern.
Should we do this in the special case?

SteveL-MSFT · 2017-09-20T23:06:49Z

@PowerShell/powershell-committee reviewed this and we should change alias for Complete to cmp

iSazonov

Leave a comment

iSazonov · 2017-09-25T11:09:08Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetVerbCommand.cs

                        verb.Verb = field.Name;
+                        verb.AliasPrefix = VerbAliasPrefixes.GetVerbAliasPrefix(field.Name);
                        verb.Group = groupName;
+                        verb.Description = VerbDescriptions.Get(field.Name);


Above we use VerbAliasPrefixes.GetVerbAliasPrefix. I believe we should use the same pattern and use VerbDescriptions.GetVerbDescription.

iSazonov · 2017-09-26T07:20:15Z

@Tadas I edit the PR description for future documentation.

mirichmo · 2017-09-27T18:14:38Z

@dantraMSFT Please re-review

dantraMSFT

I suggest you remove the VerbDescription part of the resource name in VerbDescriptionStrings.resx and avoid the string allocation in VerbDescriptions.GetVerbDescription (verb + "VerbDescription"). By definition, there should not be a name collision so the allocation is unnecessary.

dantraMSFT · 2017-09-27T19:49:40Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Verb.Tests.ps1

        }
    }
+
+    It "Should have descriptions for all verbs" {


These tests are going to be a pain to diagnose when they fail.

I think it's work taking the time to emit the missing parts instead of simply comparing the counts.

For this test, build a list of verbs that don't have a description, join the result set into a string and validate it is null or empty to output the list of verbs that are missing descriptions. I would use the same pattern with $verbsWithPrefix.

For the last test; you'll want to output the verbs that have the same alias prefix.

An alternative would be to leave the tests as is, and create a helper function that outputs the exceptions. Add a note to the test to describe how to use the function.

I'd prefer do this in place without a helper function

SteveL-MSFT · 2017-10-11T00:07:32Z

@dantraMSFT can you review again?

dantraMSFT · 2017-10-11T00:24:10Z

LGTM!

iSazonov · 2017-10-11T14:33:29Z

@Tadas Thanks for your contribution! Welcome with new ones!

Amends PowerShell#4746.

Get-Verb: add descriptions and prefixes

4094022

Tadas requested review from JamesWTruher, PaulHigin and dantraMSFT as code owners September 3, 2017 18:26

msftclas added the cla-already-signed label Sep 3, 2017

lzybkr reviewed Sep 4, 2017

View reviewed changes

dantraMSFT reviewed Sep 5, 2017

View reviewed changes

dantraMSFT suggested changes Sep 5, 2017

View reviewed changes

daxian-dbw assigned mirichmo Sep 5, 2017

Tadas added 4 commits September 8, 2017 00:29

Add description and alias prefix tests

8a63c64

Make the new classes internal

351dc4b

Convert alias prefix to static method

384df1f

Move verb descriptions to a resource file

8f98097

iSazonov reviewed Sep 8, 2017

View reviewed changes

Tadas added 4 commits September 8, 2017 13:36

Shorten function name

b46651c

Shorten alias prefix function name

14884c6

Change tests to look at counts

482094a

Add uniqueness test

f2d0aaa

Tadas added 2 commits September 8, 2017 18:32

Add missing alias prefixes

810c93f

Fix name collision

38bd60a

lzybkr reviewed Sep 17, 2017

View reviewed changes

SteveL-MSFT added Review - Committee The PR/Issue needs a review from the PowerShell Committee Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Sep 18, 2017

Change alias prefix for Compare

7bb7413

iSazonov reviewed Sep 25, 2017

View reviewed changes

Rename method

873d124

iSazonov approved these changes Sep 26, 2017

View reviewed changes

iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Sep 26, 2017

msftgits removed the cla-already-signed label Sep 26, 2017

PowerShell deleted a comment from msftclas Sep 27, 2017

dantraMSFT suggested changes Oct 4, 2017

View reviewed changes

Tadas added 2 commits October 10, 2017 22:23

Remove unnecessary string allocation

8009e8f

Make tests easier to diagnose

5d5643b

dantraMSFT approved these changes Oct 11, 2017

View reviewed changes

iSazonov approved these changes Oct 11, 2017

View reviewed changes

mirichmo merged commit 844bf11 into PowerShell:master Oct 12, 2017

Tadas deleted the add-GetVerb-descriptions branch October 23, 2017 06:43

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Get-Verb: add verb descriptions and alias prefixes' MicrosoftDocs/PowerShell-Docs#3086

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

rjmholt mentioned this pull request Jun 8, 2020

Standardize verb usage within the module microsoft/PowerShellForGitHub#228

Merged

8 tasks

yecril71pl added a commit to yecril71pl/PowerShell that referenced this pull request Jul 24, 2020

Improve inline documentation for VerbInfo

2a4759c

Amends PowerShell#4746.

yecril71pl mentioned this pull request Jul 24, 2020

Improve inline documentation for VerbInfo #13265

Merged

13 tasks


		verb.Group = groupName;

		FieldInfo descriptionField = typeof(VerbDescriptions).GetField(field.Name);

Conversation

Tadas commented Sep 3, 2017 • edited by iSazonov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

iSazonov commented Sep 4, 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!

HemantMahawar Sep 8, 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!

lzybkr commented Sep 4, 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!

Tadas commented Sep 8, 2017

Uh oh!

Tadas commented Sep 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Sep 20, 2017

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Sep 26, 2017

Uh oh!

mirichmo commented Sep 27, 2017

Uh oh!

dantraMSFT 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!

SteveL-MSFT commented Oct 11, 2017

Uh oh!

dantraMSFT commented Oct 11, 2017

Uh oh!

iSazonov commented Oct 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Tadas commented Sep 3, 2017 •

edited by iSazonov

Loading

HemantMahawar Sep 8, 2017 •

edited

Loading