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

Add argument selection handler to sample profile #1947

Merged

Conversation

ThePSAdmin
Copy link
Contributor

@ThePSAdmin ThePSAdmin commented Nov 12, 2020

PR Summary

This adds a handler to the sample PSReadLine profile which cycles through the arguments on the current command line, and selects the entire argument. This makes it simpler to quickly change an argument when recalling a command from the history, or changing the values of an argument when using a suggested command from a PSReadLine predictor plugin.

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure you've added one or more new tests
  • Make sure you've tested these changes in terminals that PowerShell is commonly used in (i.e. conhost.exe, Windows Terminal, Visual Studio Code Integrated Terminal, etc.)
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed at PowerShell-Docs
      • Doc Issue filed:
Microsoft Reviewers: Open in CodeFlow

@ghost
Copy link

ghost commented Nov 12, 2020

CLA assistant check
All CLA requirements met.

Copy link
Member

@lzybkr lzybkr left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Formatting should be consistent with the rest of the file, e.g. use 4 spaces for indent.

Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

Very interesting! The Az predictor team is actually looking for a similar experience, and we may have a built-in function in future.

@ghost ghost removed the Needs-Author Feedback label Nov 13, 2020
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

Looks good. I left 2 minor comments, once you address them we can merge the PR.

@ghost ghost removed the Needs-Author Feedback label Nov 14, 2020
@daxian-dbw
Copy link
Member

@lzybkr Could you take another look?

Copy link
Member

@lzybkr lzybkr left a comment

Choose a reason for hiding this comment

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

If there are no arguments, the cursor shouldn't move. An early return (and a Ding?) also avoids other potential issues, like indexing out of bounds (which is reported in V2 strict mode.)

I have conflicting feelings about not selecting arguments, but this is a sample, so I don't care if that behavior is changed. I say conflicting because I tried this one a command line like:

echoargs -a 1 --long 2

Repeatedly pressing Alt+a would select 1, --long, and 2. In this example, it's surprising that -a is not selected but --long is. I know why, but it does feel weird.

$args[0].Parent -is [System.Management.Automation.Language.CommandAst] -and
$args[0].Extent.StartOffset -ne $args[0].Parent.Extent.StartOffset
}, $true)

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ($asts.Count -eq 0) {
[Microsoft.PowerShell.PSConsoleReadLine]::Ding()
return
}

@ghost ghost removed the Needs-Author Feedback label Nov 18, 2020
@lzybkr lzybkr merged commit b65141e into PowerShell:master Nov 18, 2020
@ghost
Copy link

ghost commented Feb 23, 2021

🎉 v2.2.0-beta2 has been released which incorporates this pull request. 🎉

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.

3 participants