Skip to content

Add completion for Requires statements#14596

Merged
rjmholt merged 12 commits into
PowerShell:masterfrom
MartinGC94:AddRequiresCompletion
Apr 14, 2021
Merged

Add completion for Requires statements#14596
rjmholt merged 12 commits into
PowerShell:masterfrom
MartinGC94:AddRequiresCompletion

Conversation

@MartinGC94
Copy link
Copy Markdown
Contributor

@MartinGC94 MartinGC94 commented Jan 11, 2021

PR Summary

This adds completion for the various parameters and their values for #requires statements.
For example: #requires -PSEdition Core -RunAsAdministrator

The following parameters are supported:

  • Version
  • Modules
  • PSEdition
  • RunAsAdministrator

The following parameters are not supported because they have been deprecated or don't work:

  • PSSnapin
  • ShellId
  • Assembly

Most of the help text has been taken from the official docs but I took some creative liberties for the PSEditions text.

I'm not sure how strict the completion should be.
Currently it will prevent some basic mistakes like adding parameters after RunAsAdministrator but there's no special checks for randomly inserted characters so if for example you type: #requires blabla -<Tab> it will still complete to the next parameter.

PR Context

The easier the requires statements are to use, the more likely it is that people will use them. The Modules parameter in particular is hard to use without looking up the documentation due to the hashtable keys you have to remember when specifying a version.

PR Checklist

@ghost ghost assigned rjmholt Jan 11, 2021
@rjmholt rjmholt self-requested a review January 13, 2021 19:26
@rjmholt rjmholt marked this pull request as draft January 13, 2021 19:27
@rjmholt
Copy link
Copy Markdown
Collaborator

rjmholt commented Jan 13, 2021

Please convert out of draft, change the title and ping me when this PR is ready for review

@MartinGC94 MartinGC94 marked this pull request as ready for review January 15, 2021 03:22
@MartinGC94 MartinGC94 changed the title [ WIP ] Add completion for Requires statements Add completion for Requires statements Jan 15, 2021
@MartinGC94
Copy link
Copy Markdown
Contributor Author

@rjmholt ping pong, this PR is now ready for review.

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

ghost commented Jan 22, 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
Copy Markdown
Collaborator

rjmholt commented Apr 12, 2021

I was writing a review of this, but realised it would be easier (given editor assistance) to open a PR on your branch: MartinGC94#1

I've done the following things:

  • Moved the requires completions into their own method, subordinate to the if block
  • Changed some style bits, like where && and || are hung in an expression and how var is used
  • Tried to eliminate as many allocations as I can without going crazy on the readability
  • Moved some more declarative data into readonly statics
  • Changed one or two of the approaches to the actual completion itself, particularly with module spec keys

@ghost ghost removed the Review - Needed The PR is being reviewed label Apr 12, 2021
@rjmholt
Copy link
Copy Markdown
Collaborator

rjmholt commented Apr 12, 2021

One thing this PR needs though is more tests; there should essentially be a test to cover each block in the logic

@MartinGC94
Copy link
Copy Markdown
Contributor Author

Thanks for the great feedback, this is a good learning experience.
I've added more tests and made some minor changes so I hope it's good now.

@rjmholt
Copy link
Copy Markdown
Collaborator

rjmholt commented Apr 14, 2021

@MartinGC94 this is great work! Thanks so much for your contribution. Hopefully in the PR I submitted to your branch you were able to read it commit by commit to get a sense of the particular changes?

@rjmholt
Copy link
Copy Markdown
Collaborator

rjmholt commented Apr 14, 2021

The static analysis is for existing code

@rjmholt rjmholt merged commit 55d12ee into PowerShell:master Apr 14, 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
Copy Markdown

ghost commented May 27, 2021

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

Handy links:

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants