One attempt at providing a prompt function during module import. #349

rkeithhill · 2017-01-03T01:07:26Z

Fix #217. This would only export the prompt function if the prompt function is still set to the default.

I'm not suggesting we accept this just yet, just wanted to put something up folks could play with.

Fix #217. This would only export the prompt function if the prompt function is still set to the default.

@lzybkr

This was a tip from @lzybkr and it seems to solve the issue with removal of the module wiping out the prompt function. Yay!

dahlbyk · 2017-01-03T23:31:02Z

posh-git.psm1

@@ -11,26 +11,79 @@ if ($psv.Major -lt 3 -and !$NoVersionWarn) {
    "To suppress this warning, change your profile to include 'Import-Module posh-git -Args `$true'.")
 }

-Push-Location $psScriptRoot


Are we just getting lucky that nobody using PS v2 has complained about this breaking anything?

There was a pop-location later but if something crashed out before the pop-location, it would have left the current location to the module's root dir.

Oh yeah, $PSScriptRoot works only inside module (PSM1) files for PS v2 but not in PS1 files IIRC. That was fixed in v3 I believe.

Ah, that makes more sense 👍

dahlbyk · 2017-01-03T23:32:06Z

GitPrompt.ps1

 $Global:VcsPromptStatuses += $PoshGitVcsPrompt
-$ExecutionContext.SessionState.Module.OnRemove = {
-    $Global:VcsPromptStatuses = $Global:VcsPromptStatuses | Where-Object { $_ -ne $PoshGitVcsPrompt }


Why remove?

Update: Nevermind, I see it was just moved into the .psm1.

BTW I need to update that logic a bit to check if someone might have installed a different prompt function after we installed the posh-git default one.

And as a "module level" operation, posh-git.psm1 seemed like a more fitting place for this handler.

Also handle the case where the user creates a different prompt function *after* posh-git sets the prompt function. In that case, we won't restore the original prompt function that we stashed.

rkeithhill · 2017-01-04T03:35:51Z

I've done some testing on PS v2 and PS v5 both with modified prompt functions and without. It is looking pretty good and I'm feeling better about posh-git installing this when the user hasn't modified their prompt function.

BTW when the user has their own prompt function that uses posh-git commands, we don't remove it. This has the consequence of causing posh-git to be immediately reloaded when their prompt is evaluated after remove-module posh-git.

There are several workarounds including configuring a dumb prompt to allow posh-git to be removed e.g.:

function prompt { "$(pwd)> " }

or for v3 or higher

Set-Item Function:\prompt ([Runspace]::DefaultRunspace.InitialSessionState.Commands['prompt'].Definition)

They can also close all PowerShell sessions and start powershell with the -noprofile parameter. I believe this may be important in certain install situations (perhaps with Chocolatey). For PowerShellGet, each different version of posh-git will be installed into its own dir. So there shouldn't be any conflicts with updating.

dahlbyk · 2017-01-04T03:26:38Z

GitPrompt.ps1

@@ -306,8 +306,4 @@ $PoshGitVcsPrompt = {
    Write-GitStatus $GitStatus
 }

-# Install handler for removal/unload of the module
 $Global:VcsPromptStatuses += $PoshGitVcsPrompt


And as a "module level" operation, posh-git.psm1 seemed like a more fitting place for this handler.

I wonder if this should move alongside the removal operation?

If we do that, should we also move the decl/assignment of $PoshGitVcsPrompt just above this line as well?

dahlbyk · 2017-01-04T03:39:13Z

posh-git.psm1

+        # The latter is more desirable.
+        $pathInfo = $ExecutionContext.SessionState.Path.CurrentLocation
+        $curPath = if ($pathInfo.Drive) { $pathInfo.Path } else { $pathInfo.ProviderPath }
+        if ($curPath -and $curPath.ToLower().StartsWith($Home.ToLower()))


I'm a fan of the $Home check, nice addition!

Favor the StringComparison overload over comparing ToLowered strings

According to MSDN:

The string behavior of the file system, registry keys and values, and environment variables is best represented by StringComparison.OrdinalIgnoreCase.

Therefore: $curPath.StartsWith($Home, [StringComparison]::OrdinalIgnoreCase)

Commence countdown to case-insensitivity bug on Linux: 5...4...3...

We can check for Linux and use Ordinal.

dahlbyk · 2017-01-04T03:40:00Z

posh-git.psm1

+$currentPromptDef = if ($funcInfo = Get-Command prompt -ErrorAction SilentlyContinue) { $funcInfo.Definition }
+if (!$currentPromptDef -or ($currentPromptDef -eq $defaultPromptDef)) {
+    Set-Item Function:\prompt -Value {
+        $origLastExitCode = $LASTEXITCODE


PS amateur question: any reason to use $global:LASTEXITCODE here, as below?

dahlbyk · 2017-01-04T03:44:36Z

posh-git.psm1

+            $promptSuffix = "`n[DBG]: PS>> "
+        }
+        else {
+            $promptSuffix = "`nPS> "


I'd prefer not to change the default posh-git prompt since it's what non-customizing users (like me 😀) are now used to. But we could add PromptPrefix and PromptSuffix (and PromptDebug*ix?) to $GitPromptSettings to make this particular arrangement easier to achieve without modifying prompt.

OK by me. I was just floating this is an idea. If we added a setting, what would it be called? CommandEntryOnNewline?

dahlbyk · 2017-01-04T03:46:28Z

posh-git.psm1

+
+# If there is no prompt function or the prompt function is the default, export the posh-git prompt function.
+$promptReplaced = $false
+$currentPromptDef = if ($funcInfo = Get-Command prompt -ErrorAction SilentlyContinue) { $funcInfo.Definition }


Is there a reason to prefer your construction over this one?

$currentPromptDef = Get-Command prompt -ErrorAction SilentlyContinue) | Select-Object -ExpandProperty Definition

dahlbyk · 2017-01-04T03:51:10Z

posh-git.psm1

+        $promptSuffix
+    }
+
+    $promptReplaced = $true


Could we attach a property to our prompt instead of using a separate flag? I don't suppose there's any way to set its Source = "posh-git" like all the other functions we export?

dahlbyk · 2017-01-04T04:17:55Z

BTW when the user has their own prompt function that uses posh-git commands, we don't remove it. This has the consequence of causing posh-git to be immediately reloaded when their prompt is evaluated after remove-module posh-git.

Interesting... So my first thought is to have OnRemove check if the prompt it's about to restore includes Write-VcsStatus, and if so create a global Write-VcsStatus no-op on its way out (perhaps with some sort of warning that it did so). Or always create Write-VcsStatus just in case. Or let the reinstall loop happen - how dare they uninstall posh-git, anyway?

Then again, technically Write-VcsStatus is meant to be the shim through which posh-hg and friends are also exposed, so that function really shouldn't belong to posh-git at all. So we could make posh-git depend on a posh-vcs module (maintained here or moved to posh-vcs) which provides Write-VcsStatus, Un/Register-VcsStatusPrompt, and the default prompt infrastructure. Removing posh-git would then simply Unregister-VcsStatusPrompt, effectively punting the reinstall cycle to posh-vcs. I have no idea if there remains any demand for VCS support in PowerShell other than Git. (https://github.com/dahlbyk/posh-tf never took off )

Or we could hack it (Write-VcsStatus no-op or equivalent) until 1.0 at which point we retire the Write-VcsStatus cruft in favor of a posh-git prompt that calls Write-GitStatus.

rkeithhill · 2017-01-04T05:22:31Z

I hadn't quite grokked what the Vcs* functions were for yet. Thanks for the info.

Or we could hack it (Write-VcsStatus no-op or equivalent) until 1.0

But in my prompt function I use Write-GitStatus (Get-GitStatus) so this could get a bit messy. I don't think uninstall would be all that common. In fact, remove-module posh-git is likely to be rare as well. And this would only effect folks who have a customized prompt function. So I guess I'm not sure we need to worry about this for 0.7.0 other than to be aware that it could crop up from time-to-time. Plus there are simple workarounds.

dahlbyk · 2017-01-04T05:59:26Z

So how about we just log a warning OnRemove if we are restoring a non-default prompt, regardless of its contents, e.g. "If prompt still references posh-git functions, it may cause the module to re-import.")? That should be enough info to explain a reimport without overcomplicating things.

rkeithhill · 2017-01-04T06:10:08Z

Sounds good to me. I'm working on an update to this PR to address the StringComparison issue and a way to select a two line prompt. Any thoughts on the name of that setting? I'm currently using CommandEntryOnNewline but maybe it should be TwoLinePrompt?

dahlbyk · 2017-01-04T06:44:27Z

Any thoughts on the name of that setting? I'm currently using CommandEntryOnNewline but maybe it should be TwoLinePrompt?

I think we can get away with:

# Write the Git status summary information.
Write-VcsStatus

$debugMode = (Test-Path Variable:\PSDebugContext) -or [runspace]::DefaultRunspace.Debugger.InBreakpoint

$global:LASTEXITCODE = $origLastExitCode
if ($debugMode) {
    Write-Host (if ($GitPromptSettings.PromptDebugSuffix) { $GitPromptSettings.PromptDebugSuffix } else { ' [DBG]>> ' })
} else {
    Write-Host (if ($GitPromptSettings.PromptSuffix) { $GitPromptSettings.PromptSuffix } else { '> ' })
}

Your two-line prompt config becomes:

$GitPromptSettings.PromptDebugSuffix = '`n[DBG]: PS>> '
$GitPromptSettings.PromptSuffix = '`nPS> '
# plus Powerline config if applicable, which posh-git doesn't need to know

And anything more complex could be handled by a custom prompt.

rkeithhill · 2017-01-04T06:57:45Z

What if we defaulted these two settings e.g.:

$GitPromptSettings.PromptDebugSuffix = ' [DBG]: >> '
$GitPromptSettings.PromptSuffix = '> '

That would also help folks know how to use these two settings.

Change path string comparison to use [StringComparison] appropriate for the platform. Add warning in OnRemove that let's user know that their custom prompt function may cause posh-git to be re-imported. Change to use [scriptblock]::Create() otherwise $debugMode condition lied on PS v2 (although it worked fine on v5). Anyway, this change fixes this on v2. Also removed script to mess with PSReadline''s ExtraPromptLineCount.

dahlbyk

Just trivial stuff now. I'm liking where this has ended up.

Should we also drop the prompt definition from profile.example.ps1?

dahlbyk · 2017-01-04T07:24:02Z

posh-git.psm1

+        $currentPath = if ($pathInfo.Drive) { $pathInfo.Path } else { $pathInfo.ProviderPath }
+
+        # File system paths are case-sensitive on Linux and case-insensitive on Windows and macOS
+        if (($PSVersionTable.PSVersion.Major -ge 6) -and $IsLinux) {


Micro-optimization, but we could calculate $stringComparison once, assuming we can get the scriptblock to close over the variable.

dahlbyk · 2017-01-04T07:29:33Z

posh-git.psm1

    $promptReplaced = $true
 }

 # Install handler for removal/unload of the module
 $ExecutionContext.SessionState.Module.OnRemove = {
-    $Global:VcsPromptStatuses = $Global:VcsPromptStatuses | Where-Object { $_ -ne $PoshGitVcsPrompt }
+    $global:VcsPromptStatuses = $global:VcsPromptStatuses | Where-Object { $_ -ne $PoshGitVcsPrompt }

    if ($promptReplaced) {


Wondering if this flag has been made obsolete by the $promptDef -eq $poshGitPromptScriptBlock check? I suppose it's an optimization, but I wouldn't be opposed to reducing complexity at the expense of a marginally slower uninstall.

rkeithhill · 2017-01-04T07:56:24Z

Should we also drop the prompt definition from profile.example.ps1?

I think so. Perhaps we can rename that as well? I tend to following a pattern in my profile scripts of <module-name>_config.ps1. It's a practice I've ripped off from Bash.

but we could calculate $stringComparison once

The test takes between 2 & 4 mSecs and StringComparison is an enum (no heap impact). It's probably not worth bothering.

Wondering if this flag has been made obsolete

There's a small chance someone could have created a custom prompt function by copying that code. In which case, we would remove it. I could go either way.

dahlbyk · 2017-01-04T17:24:17Z

I think so. Perhaps we can rename that as well?

Well anyone using install.ps1 has a $PROFILE reference to profile.example.ps1, so we should rename/deprecate that file with prejudice. I'm inclined to defer fiddling there until after 0.7 drops when we settle on the install/import strategy for 1.0 (#328). Even then, it's probably more user-friendly for 1.0 to ship a profile.example.ps1 that simply warns that the file is obsolete and that profiles should be updated to not reference it since it will be going away with 2.0.

The test takes between 2 & 4 mSecs and StringComparison is an enum (no heap impact). It's probably not worth bothering.

👍

Wondering if this flag has been made obsolete

There's a small chance someone could have created a custom prompt function by copying that code. In which case, we would remove it. I could go either way.

I'm all for simplifying the code we have to maintain. If someone customizes their prompt poorly, that's on them - they can see all our source here.

Update to warning message in OnRemove.

rkeithhill · 2017-01-04T17:36:58Z

I'm inclined to defer fiddling there until after 0.7 drops when we settle on the install/import strategy for 1.0 (#328).

👍

Should we also drop the prompt definition from profile.example.ps1?

Should we still remove the prompt function? I'm guessing yes but wanted to double check.

rkeithhill · 2017-01-04T17:40:44Z

Also, if someone sets their $GitPromptSettings.PromptSuffix to something like $null or an empty string, PowerShell will not like that and use PS> instead e.g.:

~\GitHub\rkeithhill\posh-git [auto-export-prompt ≡]> $GitPromptSettings.PromptSuffix = ''
~\GitHub\rkeithhill\posh-git [auto-export-prompt ≡]PS>

I can put in a check for $null or empty string and then force the $promptSuffix to ' '. But should we try to save the user from themselves?

dahlbyk · 2017-01-04T17:48:26Z

Should we still remove the prompt function? I'm guessing yes but wanted to double check.

Yes.

I can put in a check for $null or empty string and then force the $promptSuffix to ' '. But should we try to save the user from themselves?

That's what motivated the extra complexity in my version - if you don't give a good override, you get the default experience. Still a good idea to set the settings properties to match the default.

if ($debugMode) {
    Write-Host (if ($GitPromptSettings.PromptDebugSuffix) { $GitPromptSettings.PromptDebugSuffix } else { ' [DBG]>> ' })
} else {
    Write-Host (if ($GitPromptSettings.PromptSuffix) { $GitPromptSettings.PromptSuffix } else { '> ' })
}

Add check for $prompSuffix null/empty.

dahlbyk · 2017-01-04T18:02:30Z

I'm feeling pretty good about this! Is there any more testing you want to do on v2 or v5 (is v3 a concern)?

rkeithhill · 2017-01-04T19:51:18Z

OK tested on v3. Works fine. I think this is ready to be merged.

rkeithhill added 3 commits January 2, 2017 17:46

One attempt to exporting a prompt function.

8908f89

Fix #217. This would only export the prompt function if the prompt function is still set to the default.

Switch to set-item to change prompt.

dd95dfe

This was a tip from @lzybkr and it seems to solve the issue with removal of the module wiping out the prompt function. Yay!

Cleanup after removing prompt func from GitPrompt.

f7525bc

rkeithhill mentioned this pull request Jan 3, 2017

Move simple prompt to the module. #217

Closed

rkeithhill changed the title ~~One attempt to exporting a prompt function.~~ One attempt at providing a prompt function during module import. Jan 3, 2017

dahlbyk reviewed Jan 3, 2017

View reviewed changes

Set PSReadlineOption for 2 line prompt.

46933f8

Also handle the case where the user creates a different prompt function *after* posh-git sets the prompt function. In that case, we won't restore the original prompt function that we stashed.

dahlbyk requested changes Jan 4, 2017

View reviewed changes

dahlbyk mentioned this pull request Jan 4, 2017

Fix that module can be found by not imported? #351

Merged

dahlbyk reviewed Jan 4, 2017

View reviewed changes

dahlbyk added this to the v0.7 milestone Jan 4, 2017

Remove unnecessary $promptReplaced variable.

cfcd365

Update to warning message in OnRemove.

Remove prompt from example.

9b54150

Add check for $prompSuffix null/empty.

dahlbyk approved these changes Jan 4, 2017

View reviewed changes

rkeithhill merged commit 464601f into master Jan 4, 2017

rkeithhill deleted the rkeithhill/auto-export-prompt branch January 4, 2017 21:21

This was referenced Jan 5, 2017

Error in $profile after poshgit installed? #355

Closed

Renamed prompt workaround #357

Merged

rkeithhill mentioned this pull request Jan 6, 2017

Posh Git Won't Show Status or Colors on Prompt (Win 7) #161

Closed

dahlbyk mentioned this pull request Jan 7, 2017

Cann't Show status in PowerShell 5.0 on Window 7 #360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

One attempt at providing a prompt function during module import. #349

One attempt at providing a prompt function during module import. #349

rkeithhill commented Jan 3, 2017

dahlbyk Jan 3, 2017

rkeithhill Jan 3, 2017

rkeithhill Jan 4, 2017

dahlbyk Jan 4, 2017

dahlbyk Jan 3, 2017 •

edited

Loading

rkeithhill Jan 4, 2017

rkeithhill Jan 4, 2017

rkeithhill commented Jan 4, 2017 •

edited

Loading

dahlbyk Jan 4, 2017

rkeithhill Jan 4, 2017

dahlbyk Jan 4, 2017

rkeithhill Jan 4, 2017

dahlbyk Jan 4, 2017

dahlbyk Jan 4, 2017

rkeithhill Jan 4, 2017

dahlbyk Jan 4, 2017

dahlbyk Jan 4, 2017

dahlbyk commented Jan 4, 2017

rkeithhill commented Jan 4, 2017

dahlbyk commented Jan 4, 2017

rkeithhill commented Jan 4, 2017

dahlbyk commented Jan 4, 2017

rkeithhill commented Jan 4, 2017

dahlbyk left a comment

dahlbyk Jan 4, 2017

dahlbyk Jan 4, 2017

rkeithhill commented Jan 4, 2017 •

edited

Loading

dahlbyk commented Jan 4, 2017

rkeithhill commented Jan 4, 2017 •

edited

Loading

rkeithhill commented Jan 4, 2017 •

edited

Loading

dahlbyk commented Jan 4, 2017

dahlbyk commented Jan 4, 2017

rkeithhill commented Jan 4, 2017

One attempt at providing a prompt function during module import. #349

One attempt at providing a prompt function during module import. #349

Conversation

rkeithhill commented Jan 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dahlbyk Jan 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill commented Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dahlbyk commented Jan 4, 2017

rkeithhill commented Jan 4, 2017

dahlbyk commented Jan 4, 2017

rkeithhill commented Jan 4, 2017

dahlbyk commented Jan 4, 2017

rkeithhill commented Jan 4, 2017

dahlbyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill commented Jan 4, 2017 • edited Loading

dahlbyk commented Jan 4, 2017

rkeithhill commented Jan 4, 2017 • edited Loading

rkeithhill commented Jan 4, 2017 • edited Loading

dahlbyk commented Jan 4, 2017

dahlbyk commented Jan 4, 2017

rkeithhill commented Jan 4, 2017

dahlbyk Jan 3, 2017 •

edited

Loading

rkeithhill commented Jan 4, 2017 •

edited

Loading

rkeithhill commented Jan 4, 2017 •

edited

Loading

rkeithhill commented Jan 4, 2017 •

edited

Loading

rkeithhill commented Jan 4, 2017 •

edited

Loading