KEMBAR78
Add comment help keyword completion by MartinGC94 · Pull Request #15337 · PowerShell/PowerShell · GitHub
Skip to content

Conversation

@MartinGC94
Copy link
Contributor

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

@MartinGC94
Copy link
Contributor Author

MartinGC94 commented May 1, 2021

It seems like my tests are failing because of the carriage returns on mac/Linux but works fine on Windows.
What's the correct way to fix this? Do I add a .Replace("`r",'') to all of the test strings and set my offset accordingly or can I do something to ensure the same line endings on all test platforms?

@vexx32
Copy link
Collaborator

vexx32 commented May 9, 2021

It seems like my tests are failing because of the carriage returns on mac/Linux but works fine on Windows.
What's the correct way to fix this? Do I add a .Replace("`r",'') to all of the test strings and set my offset accordingly or can I do something to ensure the same line endings on all test platforms?

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

@MartinGC94
Copy link
Contributor Author

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.

@ghost ghost added the Review - Needed The PR is being reviewed label May 17, 2021
@ghost
Copy link

ghost commented May 17, 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


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." },
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rjmholt Thoughts?

Copy link
Collaborator

@rjmholt rjmholt May 21, 2021

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@iSazonov
Copy link
Collaborator

@MartinGC94 Please resolve merge conflicts.

@ghost ghost removed the Review - Needed The PR is being reviewed label May 21, 2021
@MartinGC94
Copy link
Contributor Author

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.

@iSazonov
Copy link
Collaborator

iSazonov commented May 21, 2021

there doesn't seem to be a way to save changes made there

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.


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." },
Copy link
Collaborator

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.

MartinGC94 and others added 2 commits May 24, 2021 19:09
@rjmholt rjmholt merged commit 2fc3826 into PowerShell:master May 25, 2021
@daxian-dbw daxian-dbw added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label May 26, 2021
@ghost
Copy link

ghost commented May 27, 2021

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

Handy links:

@ghost ghost mentioned this pull request May 27, 2021
@MartinGC94 MartinGC94 deleted the AddCommentHelpKeywordCompletion branch June 5, 2022 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log WG-Interactive-IntelliSense tab completion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IntelliSense missing in comment blocks

5 participants