-
Notifications
You must be signed in to change notification settings - Fork 8k
Add comment help keyword completion #15337
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.
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
Add comment help keyword completion #15337
Conversation
|
It seems like my tests are failing because of the carriage returns on mac/Linux but works fine on Windows. |
One approach to this might be to avoid explicit multiline here-strings and construct the test strings like this instead: $TestString = @(
"line 1"
"line 2"
"line 3"
) -join [Environment]::NewLine |
|
Good idea about using join. I ended up using a character that could easily be removed as a fake cursor, this also makes it easier to tell where the cursor is supposed to be when looking at the test. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Robert Holt <rjmholt@gmail.com>
This reverts commit ec4f102.
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
|
|
||
| private static readonly IReadOnlyDictionary<string, string> s_commentHelpKeywords = new SortedList<string, string>(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| { "SYNOPSIS", "A brief description of the function or script. This keyword can be used only once in each topic." }, |
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.
If we really need comment strings should it be in resx file for follow localization?
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.
Maybe? PS isn't localized to my language so I don't know what the standard is but other completions use static English strings like I did here.
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.
@rjmholt Thoughts?
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.
Yeah that's a good point — these probably should be localised. @adityapatwardhan do you know if there's a good place for localised descriptions of comment-based help keywords?
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.
So my thinking is that we shouldn't block the PR on this, but before it's merged we should open an issue to track the work to use resources for this.
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 created an issue here: #15450 to clean this up in general for the completion code.
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
Co-authored-by: Ilya <darpa@yandex.ru>
|
@MartinGC94 Please resolve merge conflicts. |
|
I don't know how to resolve the merge conflict. Github has a "Resolve conflicts" button but there doesn't seem to be a way to save changes made there. I've tried creating empty space above my newly added tests to make room for the recently added test. |
Press "Mark as resolved" after you edit. Alternatively you could add the new code from main branch to your branch manually and push new commit. |
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
|
|
||
| private static readonly IReadOnlyDictionary<string, string> s_commentHelpKeywords = new SortedList<string, string>(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| { "SYNOPSIS", "A brief description of the function or script. This keyword can be used only once in each topic." }, |
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.
So my thinking is that we shouldn't block the PR on this, but before it's merged we should open an issue to track the work to use resources for this.
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Robert Holt <rjmholt@gmail.com>
|
🎉 Handy links: |
PR Summary
Fixes PowerShell/vscode-powershell#3277 by adding completion for help keywords like "DESCRIPTION" as well as their arguments like the parameter names for "PARAMETER".
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).