Skip to content

Allow CompleteInput to return results from ArgumentCompleter when AST or Script has matching function definition #10574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
adityapatwardhan merged 10 commits into from
Dec 2, 2019
Merged

Allow CompleteInput to return results from ArgumentCompleter when AST or Script has matching function definition #10574

adityapatwardhan merged 10 commits into from
Dec 2, 2019

Conversation

@M1kep
Copy link
Contributor

@M1kep M1kep commented Sep 19, 2019 •
edited by daxian-dbw
Loading

PR Summary

Fix #10567

This PR adds a check in NativeCommandArgumentCompletion to retrieve the commandName from the CommandAST if the commandName passed is null.

PR Context

When [System.Management.Automation.CommandCompletion]::CompleteInput is provided a ScriptBlock or AST that contains a definition of the function to be completed while there is a registered ArgumentCompleter for the function, the ArgumentCompleter is ignored.

This issue is visible when an editor leverages the CompleteInput to provide Intellisense(PowerShell/vscode-powershell#2162 ).

The impact to editors is that completion will be returned in such a way that better matches the terminal experience.

PR Checklist

@msftclas
Copy link

msftclas commented Sep 19, 2019 •
edited
Loading

CLA assistant check
All CLA requirements met.

@M1kep
Copy link
Contributor Author

M1kep commented Sep 19, 2019 •
edited by unfurl-links bot
Loading

I'm unsure what would cause the test to fail after simply changing a spacing issue.

Test prior to spacing issue: https://powershell.visualstudio.com/PowerShell/_build/results?buildId=32680

Test after spacing issue: https://powershell.visualstudio.com/PowerShell/_build/results?buildId=32688

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Sep 19, 2019
@iSazonov iSazonov added this to the 7.0.0-preview.5 milestone Sep 19, 2019
iSazonov
iSazonov approved these changes Sep 20, 2019
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

LGTM with one minor comment.

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Oct 7, 2019

@TylerLeonhardt for impact on vscode-powershell

@daxian-dbw - please review this change.

TylerLeonhardt
TylerLeonhardt reviewed Oct 7, 2019
View reviewed changes
daxian-dbw
daxian-dbw requested changes Oct 9, 2019
View reviewed changes
test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1
}
$pwsh = [PowerShell]::Create()
$pwsh.AddScript($scriptBl)
$pwsh.Invoke()
Copy link
Member

@daxian-dbw daxian-dbw Oct 9, 2019

Choose a reason for hiding this comment

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

This doesn't seem right. After invoking, Test-Completion would already been defined, and it's not the scenario you are aiming at -- AST or Script has matching function definition.

Copy link
Contributor Author

@M1kep M1kep Oct 9, 2019

Choose a reason for hiding this comment

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

My understanding of this is as follows:

The script block is executed to define the Test-Completion function and argument completer in the runspace.(As the runspace should provide the registered argument completers when in an editor).

From there, we are using the $script variable for completion input(Similar to how VSCode utilizes CompleteInput) as the issue arises when the function is defined in the $script block being passed to CompleteInput

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Oct 9, 2019
@M1kep M1kep requested a review from TylerLeonhardt October 9, 2019 22:08
@M1kep M1kep requested a review from daxian-dbw October 18, 2019 23:06
@adityapatwardhan
Copy link
Member

adityapatwardhan commented Oct 30, 2019

@daxian-dbw can you re-review?

M1kep and others added 2 commits October 31, 2019 00:35
daxian-dbw
daxian-dbw approved these changes Nov 7, 2019
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM assuming the comment is addressed.

@M1kep @daxian-dbw
Update src/System.Management.Automation/engine/CommandCompletion/Comp…
10de9e5 
…letionCompleters.cs

Co-Authored-By: Dongbo Wang <dongbow@microsoft.com>
@M1kep M1kep requested a review from daxian-dbw November 21, 2019 06:02
@adityapatwardhan adityapatwardhan merged commit 570ba43 into PowerShell:master Dec 2, 2019
@adityapatwardhan
Copy link
Member

adityapatwardhan commented Dec 2, 2019

@M1kep Thank you for your contribution!

@SydneyhSmith SydneyhSmith mentioned this pull request Mar 12, 2020
Closed
@ghost
Copy link

ghost commented Mar 26, 2020

🎉v7.1.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Mar 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@iSazonov iSazonov iSazonov approved these changes

@TylerLeonhardt TylerLeonhardt Awaiting requested review from TylerLeonhardt

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

+1 more reviewer

@rjmholt rjmholt rjmholt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@adityapatwardhan adityapatwardhan

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

7.1.0-preview.1

Development

Successfully merging this pull request may close these issues.

CompleteInput ignoring registered ArgumentCompleters when passed Script or AST

8 participants

@M1kep @msftclas @adityapatwardhan @daxian-dbw @TylerLeonhardt @rjmholt @iSazonov @TravisEz13