Skip to content
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

Remove redundant snippets #2062

Merged
merged 3 commits into from
Jul 10, 2019

Conversation

travisclagrone
Copy link
Contributor

@travisclagrone travisclagrone commented Jul 2, 2019

Remove the following snippets (listed by name, not prefix):

  • Cmdlet

The removed-redundant-snippet to kept-redundantee-snippet mappings are as follows (listed by name, not prefix):

  • Cmdlet : Function-Advanced
    The decision to remove Cmdlet instead of Function-Advanced is because a function defined in PowerShell with the CmdletBinding attribute is technically an advanced function rather than a cmdlet, which would be defined in C# with the Cmdlet attribute.

    However, the cmdlet prefix has also been added to the Function-Advanced snippet (whose other prefix is function-advanced), because cmdlet is an ISE snippet prefix for an advanced function and so should be preserved for compatibility.

Decision to remove 'Cmdlet' instead of 'Function-Advanced' is because
a function defined in PowerShell with the 'CmdletBinding' attribute
is technically an advanced function rather than a cmdlet, which would
be defined in C# with the 'Cmdlet' attribute.
@travisclagrone travisclagrone changed the title Remove snippet named 'Cmdlet', which is duplicate of 'Function-Advanced' Remove redundant snippets Jul 2, 2019
Copy link
Contributor

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

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

The primary reason this snippet exists is for ISE users moving to VSCode i.e. this snippet is a close approximation to the cmdlet snippet in ISE. Now, whether we want to continue to provide these "bridges" to ISE users is a question for the PS team. Just wanted to chime in with "why" this particular snippet exists.

@travisclagrone
Copy link
Contributor Author

Aha. In that case, either a more descriptive name or a source-code comment would be better.

@SydneyhSmith
Copy link
Collaborator

@travis-c-lagrone this is in two snippets because of an earlier limitation imposed by vscode , now you can update the prefix to an array to get this functionality prefix:["cmdlet", "Function-Advanced"]

@travisclagrone
Copy link
Contributor Author

@travis-c-lagrone this is in two snippets because of an earlier limitation imposed by vscode , now you can update the prefix to an array to get this functionality prefix:["cmdlet", "Function-Advanced"]

I did not know that! I might see about adding that to the VS Code documentation as well when I get the time. (it does not appear to be documented at the moment)

'cmdlet' is an ISE compatibility snippet prefix, but is identical to (and is more accurately described as) the 'function-advanced' snippet.
This is a parallel change to @58646792cd87565b9688c6123d79049abc859ddc, because--strictly speaking--comment-based help does not apply to a cmdlet but rather to an advanced function.
@travisclagrone
Copy link
Contributor Author

FYI, I plan on submitting another PR soon to have the folder .vscode settings associate the "JSON with Comments" (jsonc) language for the snippets/PowerShell.json file. This is already how VS Code interprets snippet files anyway, but it will explicitly communicate commentability within the vscode-powershell project now too. This is important in order to be able to call out (at development time) overloaded snippets with ISE compatibility prefixes, inter alia.

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for updating this!

@rjmholt rjmholt merged commit 04b0ad5 into PowerShell:master Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants