GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Enable tab complete for '-Format' parameter of Get-Date by iSazonov · Pull Request #4835 · PowerShell/PowerShell

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented Sep 14, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

Fix

  1. 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").
  2. Added a commit to replace tabs in test file.
  3. Added new ArgumentCompletionsAttribute type which allow tab completions (IntelliSense) for parameters without throw on "invalid" values in contrast to ValidateSet.
  4. Added tests for -Format parameter with named format values.
  5. 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov Ilya (iSazonov) assigned Dongbo Wang (daxian-dbw) Sep 14, 2017
@iSazonov
Ilya (iSazonov) requested review from James Truher (JamesWTruher), Paul Higinbotham (PaulHigin) and Dan Travison (dantraMSFT) as code owners September 14, 2017 10:38
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Sep 14, 2017
Ilya (iSazonov) added 4 commits September 14, 2017 15:08
@iSazonov
Replace tabs with spaces
e6aaccd
@iSazonov
Add 'AllowAll' named parameter in ValidateSet attribute
69363fd 
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
@iSazonov
Enable tab complete for '-Format' parameter of Get-Date
e142d49 
Add ValidateSet attribute to '-Format' parameter of Get-Date to enable
IntelliSense (tab complete).
@iSazonov
Add tests for Get-Date -Format named values
e7ea552
@iSazonov
Ilya (iSazonov) force-pushed the get-date-intelli branch from 8244665 to e7ea552 Compare September 14, 2017 12:24
@iSazonov
Ilya (iSazonov) requested review from Bruce Payette (BrucePay), Dongbo Wang (daxian-dbw), Jason Shirk (lzybkr) and sergei (vors) as code owners September 14, 2017 12:24
@vors

sergei (vors) commented Sep 14, 2017

Copy link
Copy Markdown
Collaborator

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Sep 14, 2017

Copy link
Copy Markdown
Collaborator Author

ValidateSet is a sealed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lzybkr
Jason Shirk (lzybkr) suggested changes Sep 14, 2017
View reviewed changes

@lzybkr Jason Shirk (lzybkr) left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/Attributes.cs Outdated
/// 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>

@lzybkr Jason Shirk (lzybkr) Sep 14, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lzybkr Jason Shirk (lzybkr) Sep 18, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be reverted.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetDateCommand.cs Outdated
/// Unix format string
/// </summary>
[Parameter(ParameterSetName = "net")]
[ValidateSetAttribute("FileDate", "FileDateUniversal", "FileDateTime", "FileDateTimeUniversal", AllowAll=true)]

@lzybkr Jason Shirk (lzybkr) Sep 14, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Sep 14, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lzybkr Jason Shirk (lzybkr) Sep 14, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Sep 15, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Ilya (iSazonov) added 2 commits September 15, 2017 15:37
@iSazonov
Enhance ArgumentCompleter to support custom string array
ab00723 
Introduce new ArgumentCompleter constructor.
Invoke the constructor in NativeCommandArgumentCompletion()
@iSazonov
Use new ArgumentCompleterAttribute in Get-Date cmdlet
52d2799
lzybkr
Jason Shirk (lzybkr) reviewed Sep 16, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetDateCommand.cs Outdated
/// </summary>
[Parameter(ParameterSetName = "net")]
[ValidateSetAttribute("FileDate", "FileDateUniversal", "FileDateTime", "FileDateTimeUniversal", AllowAll=true)]
[ArgumentCompleter("FileDate", "FileDateUniversal", "FileDateTime", "FileDateTimeUniversal")]

@lzybkr Jason Shirk (lzybkr) Sep 16, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Sep 16, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lzybkr Jason Shirk (lzybkr) Sep 17, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@powercode Staffan Gustafsson (powercode) Sep 18, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Sep 18, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done next iteration.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov
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
Ilya (iSazonov) force-pushed the get-date-intelli branch from 91b98ab to ceb9385 Compare September 18, 2017 13:10
lzybkr
Jason Shirk (lzybkr) suggested changes Sep 18, 2017
View reviewed changes

@lzybkr Jason Shirk (lzybkr) left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a little cleanup and I think it looks good.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/CommandCompletion/ExtensibleCompletion.cs Outdated
{
if (wordToCompletePattern.IsMatch(str))
{
yield return new CompletionResult(str, str, CompletionResultType.Text, str);

@lzybkr Jason Shirk (lzybkr) Sep 18, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Sep 19, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.
Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs Outdated
var argumentCompletionsAttribute = parameter.CompiledAttributes.OfType<ArgumentCompletionsAttribute>().FirstOrDefault();
if (argumentCompletionsAttribute != null)
{
try

@lzybkr Jason Shirk (lzybkr) Sep 18, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Sep 19, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/Attributes.cs Outdated
Metadata.ValidateNotNullFailure);
}

if (AllowAll == true)

@lzybkr Jason Shirk (lzybkr) Sep 18, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be reverted.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Sep 19, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/Attributes.cs Outdated
/// 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>

@lzybkr Jason Shirk (lzybkr) Sep 18, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be reverted.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/parser/Compiler.cs Outdated
{
result.IgnoreCase = s_attrArgToBoolConverter.Target(s_attrArgToBoolConverter, argValue);
}
else if (argumentName.Equals("AllowAll", StringComparison.OrdinalIgnoreCase))

@lzybkr Jason Shirk (lzybkr) Sep 18, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be reverted.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Sep 19, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Ilya (iSazonov) added 3 commits September 19, 2017 06:08
@iSazonov
Remove try and change type
d414f9b
@iSazonov
Revert "Add 'AllowAll' named parameter in ValidateSet attribute"
48e5ec5 
This reverts commit 69363fd.
@iSazonov
Fix typo in comment.
87b26b4
lzybkr
Jason Shirk (lzybkr) approved these changes Sep 19, 2017
View reviewed changes

@lzybkr Jason Shirk (lzybkr) left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Sep 19, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Ilya (@iSazonov) Can you please update the PR description? Looks like the design has changed.
And also, please consider addressing Jason Shirk (@lzybkr)'s non-blocking comment about adding tests for ArgumentCompletions.
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Ilya (iSazonov) force-pushed the get-date-intelli branch 2 times, most recently from 411123c to f86c15a Compare September 20, 2017 15:53
@iSazonov
Add tests
306fdc6
@iSazonov
Ilya (iSazonov) force-pushed the get-date-intelli branch from f86c15a to 306fdc6 Compare September 20, 2017 16:05
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Sep 20, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Sep 21, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Correct tests
1e6286e
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Sep 21, 2017
View reviewed changes
Comment thread test/powershell/Language/Parser/ExtensibleCompletion.Tests.ps1 Outdated
}

AfterAll {
#Remove-Module -ModuleInfo $testModule

@daxian-dbw Dongbo Wang (daxian-dbw) Sep 21, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you forget to uncomment this?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Sep 21, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
daxian-dbw reacted with laugh emoji
All reactions
  • 😄 1 reaction
@iSazonov
Uncomment Remove-Module
b69678c
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Sep 21, 2017
View reviewed changes
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit 93dc591 into PowerShell:master Sep 21, 2017
@iSazonov Ilya (iSazonov) added the Documentation Needed in this repo Documentation is needed in this repo label Sep 22, 2017
@iSazonov

Ilya (iSazonov) commented Sep 22, 2017

Copy link
Copy Markdown
Collaborator Author

Jason Shirk (@lzybkr) Thanks for great idea!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Ilya (iSazonov) deleted the get-date-intelli branch September 25, 2017 12:45
@daxian-dbw Dongbo Wang (daxian-dbw) mentioned this pull request Oct 4, 2017
beta.8 changelog draft #5006
Merged
@iSazonov Ilya (iSazonov) mentioned this pull request Oct 11, 2017
Unify cmdlets with parameter 'Encoding' to be of type System.Text.Encoding #5080
Merged
@joeyaiello Joey Aiello (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 Joey Aiello (joeyaiello) removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@iSazonov @daxian-dbw
Add 'ArgumentCompletionsAttribute' to support more argument completio…
d7e0606 
…n scenarios (PowerShell#4835)

- Add 'ArgumentCompletionsAttribute' to support argument completion for parameters that cannot have a ValidateSetAttribute.
- Use 'ArgumentCompletionsAttribute' for the '-Format' parameter of 'Get-Date' to enable useful argument compeltions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
@dantraMSFT Dan Travison (dantraMSFT) Awaiting requested review from dantraMSFT
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin
@BrucePay Bruce Payette (BrucePay) Awaiting requested review from BrucePay
@vors sergei (vors) Awaiting requested review from vors
+2 more reviewers
@powercode Staffan Gustafsson (powercode) powercode left review comments
@lzybkr Jason Shirk (lzybkr) lzybkr approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@daxian-dbw Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@iSazonov @vors @daxian-dbw @lzybkr @powercode @joeyaiello @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.