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

Update to prompt func to allow path/status order swap #544

Merged
merged 11 commits into from
Apr 19, 2018

Conversation

rkeithhill
Copy link
Collaborator

Plus a few other tweaks to clean up the default prompt function.

@rkeithhill
Copy link
Collaborator Author

Oops. Got some test failures to clean up.

@rkeithhill
Copy link
Collaborator Author

This would allow folks to more fully customize the default prompt. This is one example:

$settings = $global:GitPromptSettings
$settings.DefaultPromptAbbreviateHomeDirectory = $true
$settings.DefaultPromptWriteStatusFirst = $true
$settings.BeforeText.Text = '['
$settings.AfterText.Text  = '] '
$settings.DefaultPromptTimingFormat = "{0}ms:"
$settings.DefaultPromptPath.Text += "`n"
$settings.DefaultPromptDebug = "[DBG]: "
$settings.DefaultPromptDebug.ForegroundColor = [ConsoleColor]::Magenta
$settings.DefaultPromptSuffix.Text = '$(if ($hist = Get-History -Count 1) {$hist.id+1} else {1})' + $settings.DefaultPromptSuffix.Text

To get this form of prompt:

[rkeithhill/change-debugprompt ≡] ~\GitHub\dahlbyk\posh-git
180ms:4>


[PoshGitTextSpan]$DefaultPromptPrefix = ''
[PoshGitTextSpan]$DefaultPromptSuffix = '$(''>'' * ($nestedPromptLevel + 1)) '
[PoshGitTextSpan]$DefaultPromptDebugSuffix = ' [DBG]$(''>'' * ($nestedPromptLevel + 1)) '
Copy link
Collaborator Author

@rkeithhill rkeithhill Feb 10, 2018

Choose a reason for hiding this comment

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

FYI the duplication here of the $nestedPromptLevel code bothered me. So now we display in order (where [] designates optional text:

[ DPDebugSuffix][ DPTimingFormat]DPSuffix

[bool]$DefaultPromptAbbreviateHomeDirectory = $false

[bool]$DefaultPromptEnableTiming = $false
[PoshGitTextSpan]$DefaultPromptTimingFormat = ' {0}ms'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't envision too many folks using prompt timing at all. But for those of us that do, I prefer not to have the ms in there. This is pretty easy way to accomplish this.

@rkeithhill rkeithhill force-pushed the rkeithhill/change-debugprompt branch from c6fa372 to ec0318a Compare February 12, 2018 02:41
@@ -141,12 +141,12 @@ class PoshGitTextSpan {
}
else {
$bg = $this.BackgroundColor
if ($bg -and !(Test-VirtualTerminalSequece $bg)) {
if (($null -ne $bg) -and !(Test-VirtualTerminalSequece $bg)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI this allows you to set the value to 0 (black). Before, it would see the 0 as false in this condition and use the "default" color.

@rkeithhill
Copy link
Collaborator Author

@dahlbyk I think this is ready to go. OK if I merge it?

@rkeithhill
Copy link
Collaborator Author

rkeithhill commented Feb 18, 2018

A few more changes here that I'm proposing. I have been noodling on how to make it simpler to switch path/status order (fewer settings requiring tweaks) and how to make it simpler to switch to a two-line prompt.

For this, I think it is helpful to see the current and proposed prompt layout. Note [] represent parts of the prompt that may not always be displayed:

Current prompt layout:

Default config:
{DPPrefix}{DPPath}[{BeforeText}{Status}{AfterText}][{DPDebug}][{DPTimingFormat}]{DPSuffix}

Status first config:
{DPPrefix}[{BeforeText}{Status}{AfterText}]{DPPath}[{DPDebug}][{DPTimingFormat}]{DPSuffix}


Proposed change:

Default config:
{DPPrefix}{DPPath}{PathStatusSep}[{BeforeStatus}{Status}{AfterStatus}]{DPMiddle}[{DPDebug}][{DPTimingFormat}]{DPSuffix}

Status first config:
{DPPrefix}[{BeforeStatus}{Status}{AfterStatus}]{PathStatusSep}{DPPath}{DPMiddle}[{DPDebug}][{DPTimingFormat}]{DPSuffix}

The above changes (corresponding to the commit I'm getting ready to push, gives us some nice flexibilty.

Is someone wants to swap path/status, then they simply set (no need to fiddle with both BeforeStatus and AfterStatus):

$global:GitPromptSettings.DefaultPromptWriteStatusFirst = $true

BTW I've rename a few of the stettings that had Text in the name because it was really weird to see BeforeText.Text = '[' and likely misleading as well. That is, folks would be tempted to assign like so BeforeText = '[' which works but wipes out any previously set fg/bg colors.

If they want a two line prompt:

$global:GitPromptSettings.DefaultPromptMiddle.Text = '`n'

I also have the default prompt, when in ANSI mode, saving the escaped prompt string to the global variable PoshGitLastPrompt. I figure this will come in handy in debug scenarios. Here's an example of what that looks like on PS Core:

~\GitHub\rkeithhill\PowerShell [master+1 ~0 -0 !] [DBG]:>> $PoshGitLastPrompt
~\GitHub\rkeithhill\PowerShell `e[93m[`e[0m`e[96mmaster`e[0m`e[96m`e[0m`e[31m`e[49m +1`e[0m`e[31m`e[49m ~0`e[0m`e[31m`e[49m -0`e[0m`e[31m !`e[0m`e[93m]`e[0m`e[95m [DBG]:`e[0m>>

@rkeithhill
Copy link
Collaborator Author

rkeithhill commented Feb 18, 2018

FYI, this is the prompt config I'm currently running with:

$global:GitPromptSettings.DefaultPromptWriteStatusFirst = $true
$global:GitPromptSettings.DefaultPromptMiddle.Text = '`n$([DateTime]::now.ToString("MM-dd HH:mm:ss"))'
$global:GitPromptSettings.DefaultPromptMiddle.ForegroundColor = 0x7f7f7f
$global:GitPromptSettings.DefaultPromptEnableTiming = $true
$GitPromptSettings.DefaultPromptSuffix = ' $((Get-History -Count 1).id + 1)$(">" * ($nestedPromptLevel + 1)) '

This gives me:
image
Note: the number right before the > char is the history id number.

@rkeithhill rkeithhill mentioned this pull request Feb 20, 2018
13 tasks
@rkeithhill rkeithhill force-pushed the rkeithhill/change-debugprompt branch 2 times, most recently from 9d4465c to 841eb45 Compare March 3, 2018 05:39
@rkeithhill rkeithhill requested a review from dahlbyk March 10, 2018 22:44
@rkeithhill
Copy link
Collaborator Author

@dahlbyk Any objections if I go ahead and merge this? Since it is still beta it would be good to get some feedback on the DefaultPrompt setting changes.

@dahlbyk dahlbyk force-pushed the rkeithhill/change-debugprompt branch from 841eb45 to 5d70345 Compare April 17, 2018 03:03
Copy link
Owner

@dahlbyk dahlbyk left a comment

Choose a reason for hiding this comment

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

So, first and foremost: sorry this sat unanswered for two months. 😭

That said, I love the changes! I squashed up a few of the commits to reduce the code churn a bit, and left a few questions/comments.

And, of course, the build failed with "Unable to locate package powershell".

Note: the number right before the > char is the history id number.

Man, I did something like this forever ago and completely forgot about it. 🥂

[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssigments', '')]
$nonRepoRegex = '^PowerShell \d+\.\d+\.\d+(\.\d+|-\S+)? \(\d+\)$'
$nonRepoRegex = '^PowerShell \d+\.\d+\.\d+(\.\d+|-\S+)? \d\d-bit \(\d+\)$'
Copy link
Owner

Choose a reason for hiding this comment

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

These are going to break when we get to 128-bit processors. :trollface:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed. :-)

$res = [string](&$prompt 6>&1)
$res | Should BeExactly "${env:HOME}> "
$res = &$prompt
"$res" | Should BeExactly "${env:HOME}> "
Copy link
Owner

Choose a reason for hiding this comment

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

Can you help me understand why sometimes you use [string](&$prompt 6>&1) and others you use this version?

@@ -259,6 +260,7 @@ class PoshGitPromptSettings {

[PoshGitTextSpan]$DefaultPromptPrefix = ''
[PoshGitTextSpan]$DefaultPromptPath = '$(Get-PromptPath)'
[PoshGitTextSpan]$DefaultPromptMiddle = ''
Copy link
Owner

Choose a reason for hiding this comment

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

Middle's a bit vague... maybe DefaultPromptBeforeSuffix?

Copy link
Collaborator Author

@rkeithhill rkeithhill Apr 17, 2018

Choose a reason for hiding this comment

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

Yeah, Middle is a bit vague. OTOH this section isn't always before the suffix i.e. if Debug or Timing is enabled this text appears before that. That said, in most cases (including the default) it is before the suffix.

Copy link
Owner

Choose a reason for hiding this comment

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

I guess I was thinking of Debug/Timing as part of the suffix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can make that change but not tonight - getting late and I have VSCode-powershell PR to finish up. I can get to this tomorrow though as I really would like to get this in for beta 2. For somewhat selfish reasons I'm excited about this PR as it allows me to not have to define a custom prompt function anymore. :-)

# Write default prompt middle if not empty.
if ($settings.DefaultPromptMiddle.Text) {
$promptMiddle = [PoshGitTextSpan]::new($settings.DefaultPromptMiddle)
$promptMiddle.Text = $ExecutionContext.SessionState.InvokeCommand.ExpandString($promptMiddle.Text)
Copy link
Owner

Choose a reason for hiding this comment

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

We do this enough, I wonder if it would be worth supporting [PoshGitTextSpan]::Expand($settings.DefaultPromptMiddle)? (Or add [bool]$expand = $false to the ctor?)

If we include the empty check, we could replace this whole block with just...

$prompt += Write-Prompt [PoshGitTextSpan]::Expand($settings.DefaultPromptMiddle)

Copy link
Collaborator Author

@rkeithhill rkeithhill Apr 17, 2018

Choose a reason for hiding this comment

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

That is acting kind of like a ctor. Maybe we could add a parameter to the ctor e.g.:

$prompt += Write-Prompt [PoshGitTextSpan]::new($settings.DefaultPromptMiddle, $ExecutionContext)

Or perhaps even better (shorter to type anyway):

$prompt += Write-Prompt $settings.DefaultPromptMiddle.Expand($ExecutionContext)

The Expand() method is sort of a fluent API that returns a copy of the original textspan but with the text expanded. Note: we have to pass in the $ExecutionContext or technically the $ExecutionContext.SessionState.InvokeCommand object but the former is less to type. :-) We have to do this because the context isn't available inside an instance of a PowerShell class.

I've tested the second approach above and it works fine.

Copy link
Owner

Choose a reason for hiding this comment

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

I like the "extension method" approach the best.

Note: we have to pass in the $ExecutionContext…because the context isn't available inside an instance of a PowerShell class.

TIL

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cool. BTW poked around and found a way to get the $ExecutionContext inside the Expand() method so we won't have to pass that in after all. Turns out PS classes can't find this "automatic" variable when it is compiled but you can work-around it by asking for the variable at runtime with Get-Variable ExecutionContext -ValueOnly. So we should be able to simplify it to just:

$prompt += Write-Prompt $settings.DefaultPromptMiddle.Expand()

Copy link
Collaborator Author

@rkeithhill rkeithhill Apr 18, 2018

Choose a reason for hiding this comment

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

I've got a fix for both the DefaultPromptBeforeSuffix issue and Expand but with the branch updates on the server, I can no longer push my changes. Can we merge this then I'll submit another PR with those two changes? Or do you know the magic Git command to get my local branch in sync? I suppose I could delete it and check it out again from the server.

Copy link
Owner

Choose a reason for hiding this comment

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

Assuming origin is your remote for this repo, and you have two new local commits since I rebased…

git fetch
git rebase HEAD~2 --onto origin/rkeithhill/change-debugprompt

@dahlbyk dahlbyk added this to the v1.0 milestone Apr 17, 2018
@rkeithhill
Copy link
Collaborator Author

Thanks for the rebase help. I "think" I've addressed all your PR concerns.

Copy link
Owner

@dahlbyk dahlbyk left a comment

Choose a reason for hiding this comment

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

LGTM. Please review the last two commits I added, then I think we're done.

Can you help me understand why sometimes you use [string](&$prompt 6>&1) and others you use this version?

Eh?

@dahlbyk dahlbyk force-pushed the rkeithhill/change-debugprompt branch from 4df237a to 30a888e Compare April 18, 2018 18:51
Copy link
Owner

@dahlbyk dahlbyk left a comment

Choose a reason for hiding this comment

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

Let's pretend I didn't just break the build.

Am I supposed to be using a specific version of Pester? I'm on a new machine that apparently came with Pester 3.4, but posh-git tests are failing. pester/Pester#585

@dahlbyk dahlbyk force-pushed the rkeithhill/change-debugprompt branch from 30a888e to 763368b Compare April 18, 2018 18:57
Copy link
Owner

@dahlbyk dahlbyk left a comment

Choose a reason for hiding this comment

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

Am I supposed to be using a specific version of Pester? I'm on a new machine that apparently came with Pester 3.4, but posh-git tests are failing.

Answered my own question: -MinimumVersion 4.0.8

@@ -61,9 +61,7 @@ $GitPromptScriptBlock = {
$prompt = ''

# Write default prompt prefix if not empty.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Guess we should change the comment to just # Write default prompt prefix.

@@ -79,12 +77,10 @@ $GitPromptScriptBlock = {
}

# Write default prompt middle if not empty.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ditto - drop the if not empty bit.

return $StringBuilder
}

return ""
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could probably simplify this to a one-liner: return $(if ($StringBuilder) {$StringBuilder} else {""})

Copy link
Owner

Choose a reason for hiding this comment

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

That would be fine. I'm crashing for the night, but if you want to patch these up I can merge in the morning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do.

@rkeithhill
Copy link
Collaborator Author

rkeithhill commented Apr 19, 2018

RE:

Can you help me understand why sometimes you use [string](&$prompt 6>&1) and others you use this version?

Looks like I missed one in an ANSI test. In general, $res = [string](&$prompt *>&1) is used in all the NO ANSI cases so we get host information stream (all of them actually) redirected to stdout so we can capture the prompt string that was written using Write-Host. In the ANSI cases, there is only stdout output (string returned by the StringBuilder) so I simply use $res = &$prompt.

Want me to fix it?

@dahlbyk
Copy link
Owner

dahlbyk commented Apr 19, 2018

Want me to fix it?

I'm content for now as long as the tests are green. I do wonder about hiding some of the prompt-as-string complexity in a function so we don't have to touch all the tests if the patteen changes. Something to ponder.

rkeithhill and others added 2 commits April 18, 2018 21:08
Also modified ANSI tests to be consistent and not flatten $res with
string interpolation.  Catch more errors this way.
@dahlbyk dahlbyk merged commit 554b881 into develop Apr 19, 2018
@dahlbyk dahlbyk deleted the rkeithhill/change-debugprompt branch April 19, 2018 14:27
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.

2 participants