8000 Improve completion of parameters for attributes by MartinGC94 · Pull Request #14525 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Improve completion of parameters for attributes #14525

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
iSazonov merged 3 commits into from
Jan 14, 2021
Merged

Improve completion of parameters for attributes #14525

iSazonov merged 3 commits into from
Jan 14, 2021

Conversation

@MartinGC94
Copy link
Contributor
@MartinGC94 MartinGC94 commented Dec 31, 2020

PR Summary

Fixes: #12771
Fixes issue where tab completion for attribute parameters only works for the first parameter due to the comma(s) required for the following parameters.

PR Context

If you are using an attribute like [Parameter()] you need to remember the full list of parameters because tab completion/intellisense doesn't work unless you are writing the first parameter.

PR Checklist

@ghost ghost assigned iSazonov Dec 31, 2020
@iSazonov
Copy link
Collaborator
iSazonov commented Jan 1, 2021

I'd expect follow works but it doesn't:

function bar { [parameter(Position,<Tab>

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jan 1, 2021
@MartinGC94
Copy link
Contributor Author
MartinGC94 commented Jan 1, 2021

Agreed, that would be nice but I don't know how I would implement that.
Incomplete input like [ValidateSet( doesn't get any ASTs of type AttributeAst and the completionContext doesn't contain the tokens that would otherwise let me know about an attribute.

Do I update completionContext to also include the tokens? Do I parse the input again to get the tokens? Use regex to determine if the text looks like an attribute?

@iSazonov
Copy link
Collaborator
iSazonov commented Jan 1, 2021 •
edited
Loading

Agreed, that would be nice but I don't know how I would implement that.

Yes, it would be nice. I don't know if even that is possible (all we have is an ErrorAst). It would be a sad limitation. If so this could be addressed by prediction feature (not in the PR).
Perhaps @daxian-dbw and @rjmholt could advice something.
Perhaps it is useful for PES.

@ghost ghost added the Review - Needed The PR is being reviewed label Jan 13, 2021
@ghost
Copy link
ghost commented Jan 13, 2021

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@rjmholt
Copy link
Collaborator
rjmholt commented Jan 13, 2021

Agreed, that would be nice but I don't know how I would implement that.
Incomplete input like [ValidateSet( doesn't get any ASTs of type AttributeAst and the completionContext doesn't contain the tokens that would otherwise let me know about an attribute.

Do I update completionContext to also include the tokens? Do I parse the input again to get the tokens? Use regex to determine if the text looks like an attribute?

Definitely better in a separate PR.

I don't have a total picture of how to achieve this, but:

  • Regex should not be used -- the tokeniser is built for this purpose
  • The completer should probably have as much information as exists in order to generate good completions. This includes tokens.
  • You may encounter complexity with composite tokens, which are ones that in my opinion the tokeniser doesn't handle very well. But you should still have enough information in the token string to do the right thing given enough determination.

I believe the current token is available here:

PowerShell/src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs

Lines 22 to 24 in c909541

internal Token TokenAtCursor { get; set; }
internal Token TokenBeforeCursor { get; set; }

and the full token array is available here:

PowerShell/src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs

Line 62 in c909541

private readonly Token[] _tokens;

It should be possible to search backward through the token array if need be.

If the token up to that point isn't available, I don't think there's a way of piercing through the tokeniser abstraction to see the raw input currently, but the right way to do that would be to expose it as a token anyway.

@ghost ghost removed the Review - Needed The PR is being reviewed label Jan 13, 2021
@iSazonov @rjmholt
Update src/System.Management.Automation/engine/CommandCompletion/Comp…
328ba08 
…letionAnalysis.cs

Co-authored-by: Robert Holt <rjmholt@gmail.com>
@iSazonov iSazonov merged commit e26d708 into PowerShell:master Jan 14, 2021
@iSazonov iSazonov added this to the 7.2.0-preview.3 milestone Jan 14, 2021
@iSazonov
Copy link
Collaborator
iSazonov commented Jan 14, 2021

@MartinGC94 Thanks for your contribution!

@ghost
Copy link
ghost commented Feb 12, 2021

🎉v7.2.0-preview.3 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Feb 12, 2021
Closed
@MartinGC94 MartinGC94 deleted the Improve-completion-for-attributes branch June 5, 2022 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

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

+1 more reviewer

@rjmholt rjmholt rjmholt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov iSazonov

Labels

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

Projects

None yet

Milestone

7.2.0-preview.3

Development

Successfully merging this pull request may close these issues.

Intellisense only works for the first parameter of an attribute

3 participants

@MartinGC94 @iSazonov @rjmholt
0