Enable tab complete for '-Format' parameter of Get-Date by iSazonov · Pull Request #4835 · PowerShell/PowerShell

iSazonov · 2017-09-14T10:38:27Z

Add ArgumentCompletions attribute to '-Format' parameter of Get-Date to enable
IntelliSense (tab complete).

Fix

Added ArgumentCompletions attribute to enable IntelliSense but allow not only named format values ("FileDate", "FileDateUniversal", "FileDateTime", "FileDateTimeUniversal") but also custom format strings ("YYYY-MM-dd").
Added a commit to replace tabs in test file.
Added new ArgumentCompletionsAttribute type which allow tab completions (IntelliSense) for parameters without throw on "invalid" values in contrast to ValidateSet.
Added tests for -Format parameter with named format values.
Added tests for ArgumentCompletionsAttribute

Additional considerations

No tab completion test added for Get-Date because it seems we have no requirements to check tab completion for every parameter in all cmdlets.

If AllowAll==true 'ValidateElement' always pass all elements. Useful if we need enable IntelliSense (TabComplete) but effectively disable validation when a parameter can accept not only the listed values but also arbitrary. Ex.: Get-Date -Format

Add ValidateSet attribute to '-Format' parameter of Get-Date to enable IntelliSense (tab complete).

vors · 2017-09-14T15:42:05Z

Should we use another attribute name just for the IntelliSense scenario?
[ValidateSet(AllowAll)] looks cumbersome. We can inherit from ValidateSetAttribute and use some name that doesn't mention Validation. It's also not very clean, but at least for the end user it will be not as confusing imo.

iSazonov · 2017-09-14T16:42:54Z

ValidateSet is a sealed.

lzybkr

Thanks for thinking about the usability here - I agree we should do something here, just not like this.

lzybkr · 2017-09-14T17:56:17Z

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

+        /// Useful if we need enable IntelliSense (TabComplete) but effectively disable validation
+        /// when a parameter can accept not only the listed values but also arbitrary.
+        /// Ex.: Get-Date -Format
+        /// </summary>


This doesn't feel right to me - you shouldn't use a Validation attribute if you aren't validating.

This should be reverted.

lzybkr · 2017-09-14T17:58:08Z

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

        /// Unix format string
        /// </summary>
        [Parameter(ParameterSetName = "net")]
+        [ValidateSetAttribute("FileDate", "FileDateUniversal", "FileDateTime", "FileDateTimeUniversal", AllowAll=true)]


I would suggest using an ArgumentCompleter attribute instead - it's a bit more cumbersome, but if this scenario makes sense (and I think it probably does), we can find some way to make it simpler.

Do you suggest enhance ArgumentCompleter to support [ArgumentCompleter("FileDate", "FileDateUniversal", "FileDateTime", "FileDateTimeUniversal")] ?

Just brainstorming here - but maybe a new attribute ArgumentCompletions that also implements IArgumentCompleter - this probably requires some additional code elsewhere, probably similar to the ValidateSet code for completion.

A new attribute requires a lot of code. It seems easiest to add a new constructor public ArgumentCompleterAttribute(params string[] completeStrings)

Introduce new ArgumentCompleter constructor. Invoke the constructor in NativeCommandArgumentCompletion()

lzybkr · 2017-09-16T16:55:55Z

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

        /// </summary>
        [Parameter(ParameterSetName = "net")]
-        [ValidateSetAttribute("FileDate", "FileDateUniversal", "FileDateTime", "FileDateTimeUniversal", AllowAll=true)]
+        [ArgumentCompleter("FileDate", "FileDateUniversal", "FileDateTime", "FileDateTimeUniversal")]


This is much better, but I think still a little weird, I still think it's worth considering adding a new attribute ArgumentCompletions. It seems like you could take your class StringArrayArgumentCompleter, rename it, make it an attribute, and either duplicate a little bit of logic around ValidateSet or ArgumentCompleter in the tab completion code.

Is your only thought that ArgumentCompleter is a strange name and you want to rename it?

I wouldn't rename ArgumentCompleter even if it is a strange name (which I suppose it is.)

It's a really minor thing, but to me, ArgumentCompleter implies some dynamic code to compute the completions, whereas ArgumentCompletions is static.

I have missed that for a long time - the ability to easily complete a set without limiting the parameter to the completions.

Is it a reasonable compromise that you have to go IArgumentCompleter if you want to provide rich CompletionResults, or should be enable this with ArgumentCompletions too?

Done next iteration.

This attribute is used to specify an argument completions for a parameter of a cmdlet or function without force throw unlike ValidateSetAttribute.

lzybkr

Just a little cleanup and I think it looks good.

lzybkr · 2017-09-18T20:31:33Z

src/System.Management.Automation/engine/CommandCompletion/ExtensibleCompletion.cs

+            {
+                if (wordToCompletePattern.IsMatch(str))
+                {
+                    yield return new CompletionResult(str, str, CompletionResultType.Text, str);


Should this be CompletionResultType.ParameterValue? I think that's the only place the attribute will have any effect.

I agree.
Fixed.

lzybkr · 2017-09-18T20:32:15Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs

+            var argumentCompletionsAttribute = parameter.CompiledAttributes.OfType<ArgumentCompletionsAttribute>().FirstOrDefault();
+            if (argumentCompletionsAttribute != null)
+            {
+                try


You are calling internal code which doesn't throw, so the try/catch is unnecessary.

lzybkr · 2017-09-18T20:32:29Z

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

                            Metadata.ValidateNotNullFailure);
            }

+            if (AllowAll == true)


This should be reverted.

lzybkr · 2017-09-18T20:32:36Z

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

+        /// Useful if we need enable IntelliSense (TabComplete) but effectively disable validation
+        /// when a parameter can accept not only the listed values but also arbitrary.
+        /// Ex.: Get-Date -Format
+        /// </summary>


This should be reverted.

lzybkr · 2017-09-18T20:32:57Z

src/System.Management.Automation/engine/parser/Compiler.cs

                {
                    result.IgnoreCase = s_attrArgToBoolConverter.Target(s_attrArgToBoolConverter, argValue);
                }
+                else if (argumentName.Equals("AllowAll", StringComparison.OrdinalIgnoreCase))


This should be reverted.

This reverts commit 69363fd.

lzybkr

Code changes are approved - but we should get a couple of tests for the new ArgumentCompletions attribute.

daxian-dbw · 2017-09-19T20:11:23Z

@iSazonov Can you please update the PR description? Looks like the design has changed.
And also, please consider addressing @lzybkr's non-blocking comment about adding tests for ArgumentCompletions.

daxian-dbw · 2017-09-20T16:27:17Z

@iSazonov The tests look good, thanks!
Hmm, there are failures in AppVeyor test run, can you please take a look?

iSazonov · 2017-09-21T03:47:35Z

~~Could you please help: C# test failed on "ArgumentCompleter" abbreviation - full name ArgumentCompleterAttribute works. I cannot find how make "ArgumentCompleter" alias.~~

daxian-dbw · 2017-09-21T06:23:24Z

test/powershell/Language/Parser/ExtensibleCompletion.Tests.ps1

+    }
+
+    AfterAll {
+        #Remove-Module -ModuleInfo $testModule


Did you forget to uncomment this?

Yes 😄 Today tests is passed. I wonder - I don't change a code.

iSazonov · 2017-09-22T05:05:42Z

@lzybkr Thanks for great idea!

iSazonov assigned daxian-dbw Sep 14, 2017

iSazonov requested review from JamesWTruher, PaulHigin and dantraMSFT as code owners September 14, 2017 10:38

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

iSazonov added 4 commits September 14, 2017 15:08

Replace tabs with spaces

e6aaccd

Enable tab complete for '-Format' parameter of Get-Date

e142d49

Add ValidateSet attribute to '-Format' parameter of Get-Date to enable IntelliSense (tab complete).

Add tests for Get-Date -Format named values

e7ea552

iSazonov force-pushed the get-date-intelli branch from 8244665 to e7ea552 Compare September 14, 2017 12:24

iSazonov requested review from BrucePay, daxian-dbw, lzybkr and vors as code owners September 14, 2017 12:24

lzybkr suggested changes Sep 14, 2017

View reviewed changes

iSazonov added 2 commits September 15, 2017 15:37

Enhance ArgumentCompleter to support custom string array

ab00723

Introduce new ArgumentCompleter constructor. Invoke the constructor in NativeCommandArgumentCompletion()

Use new ArgumentCompleterAttribute in Get-Date cmdlet

52d2799

lzybkr reviewed Sep 16, 2017

View reviewed changes

Add ArgumentCompletionsAttribute

ceb9385

This attribute is used to specify an argument completions for a parameter of a cmdlet or function without force throw unlike ValidateSetAttribute.

iSazonov force-pushed the get-date-intelli branch from 91b98ab to ceb9385 Compare September 18, 2017 13:10

lzybkr suggested changes Sep 18, 2017

View reviewed changes

iSazonov added 3 commits September 19, 2017 06:08

Remove try and change type

d414f9b

Revert "Add 'AllowAll' named parameter in ValidateSet attribute"

48e5ec5

This reverts commit 69363fd.

Fix typo in comment.

87b26b4

lzybkr approved these changes Sep 19, 2017

View reviewed changes

iSazonov force-pushed the get-date-intelli branch 2 times, most recently from 411123c to f86c15a Compare September 20, 2017 15:53

Add tests

306fdc6

iSazonov force-pushed the get-date-intelli branch from f86c15a to 306fdc6 Compare September 20, 2017 16:05

Correct tests

1e6286e

daxian-dbw reviewed Sep 21, 2017

View reviewed changes

Uncomment Remove-Module

b69678c

daxian-dbw approved these changes Sep 21, 2017

View reviewed changes

daxian-dbw merged commit 93dc591 into PowerShell:master Sep 21, 2017

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

iSazonov deleted the get-date-intelli branch September 25, 2017 12:45

daxian-dbw mentioned this pull request Oct 4, 2017

beta.8 changelog draft #5006

Merged

iSazonov mentioned this pull request Oct 11, 2017

Unify cmdlets with parameter 'Encoding' to be of type System.Text.Encoding #5080

Merged

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Enable tab complete for '-Format' parameter of Get-Date' MicrosoftDocs/PowerShell-Docs#3084

Closed

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

Conversation

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

Uh oh!

Fix

Additional considerations

Uh oh!

vors commented Sep 14, 2017

Uh oh!

iSazonov commented Sep 14, 2017

Uh oh!

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

powercode Sep 18, 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!

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

lzybkr left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

iSazonov commented Sep 22, 2017

Uh oh!

Reviewers

Assignees

iSazonov commented Sep 14, 2017 •

edited

Loading

powercode Sep 18, 2017 •

edited

Loading

daxian-dbw commented Sep 19, 2017 •

edited

Loading

daxian-dbw commented Sep 20, 2017 •

edited

Loading

iSazonov commented Sep 21, 2017 •

edited

Loading

iSazonov Sep 21, 2017 •

edited

Loading