Refactor Settings & Write-GitStatus function #513

rkeithhill · 2017-12-29T06:53:41Z

This builds on PR #499 (refactor settings) by refactoring Write-GitStatus into several smaller parts that can be called individually.

Also of interest is the added support for 24-bit color e.g. `$GitPromptSettings.BranchColor.ForegroundColor = 0xFFA000

@dahlbyk At the very least, we should consider applying the part that adds support for 24-bit color and move ConsoleColor strings ahead of HTML color strings. PS Core 6.0.0 GA is set for mid-January. It would be nice to at least get a beta of posh-git 1.0.0 into the PSGallery.. Oh yeah, PSGallery finally supports pre-release modules - https://blogs.msdn.microsoft.com/powershell/2017/12/05/prerelease-versioning-added-to-powershellget-and-powershell-gallery/

NOTE: this PR and #499 uses PS classes for perf reasons (using Add-Type C# code was too slow on Linux) so we would need to set the min PS version in the PSD1 file to 5.0. I think this would be OK if we pitch this release as primarily support for PS Core and PS on Windows 10 conhost w/VT seq functionality.

Not sure whether all these individual methods should be exposed but I'm inclined to expose them so folks can compose their Git status in their own order.

This turns out to be important when you've changed the structure of the settings. For instance, setting $GitPromptSettings.BranchColor = [consolecolor]::red doesn't work as expected because BranchColor is supposed to be a [PoshGitCellColor] object. With the strongly typed settings, the above assignement will generate an error. The assignment should be $GitPromptSettings.BranchColor.ForegroundColor = [ConsoleColor]::.

rkeithhill · 2017-12-31T07:25:02Z

src/posh-git.psd1

+    'Write-GitBranchStatus',
+    'Write-GitIndexStatus',
+    'Write-GitStashCount',
+    'Write-GitWorkingDirStatus',


Maybe get rid of Dir in this name?

How about combining Write-GitIndexStatus, Write-GitWorkingDirStatus and Write-GitWorkingDirLocalStatus as Write-GitFileStatus, named in alignment with EnableFileStatus? Then instead of checking the flag outside the function, we'd check it once inside the function and return early if appropriate.

Rename Write-GitWorkingDirLocalStatus to Write-GitWorkingDirStatusSummary Add -NoLeadingSpace parameter to individual Write-Git* functions to control output of leading space.

ModuleVersion must use semver - major.minor.patch (no fourth part). Set Prerelease to `-beta1` per instructions on https://blogs.msdn.microsoft.com/powershell/2017/12/05/prerelease-versioning-added-to-powershellget-and-powershell-gallery/ Update copyright date to 2018.

rkeithhill · 2018-01-01T20:46:44Z

src/AnsiUtils.ps1

@@ -24,29 +24,60 @@ $AnsiEscape = [char]27 + "["
 $ColorTranslatorType = 'System.Drawing.ColorTranslator' -as [Type]
 $ColorType = 'System.Drawing.Color' -as [Type]

+function EscapseAnsiString([string]$AnsiString) {


FYI, this function is used in the ToString() functions to display the original ANSI seq e.g.:

Allow advanced users to invoke the $scriptblock from their own prompt functions.

dahlbyk

First pass of feedback... sorry I didn't take the time to review #499 a few months ago. 😬

dahlbyk · 2018-01-01T20:54:15Z

src/AnsiUtils.ps1

+    $res
+}
+
+function Test-VirtualTerminalSequece([psobject]$Object) {


Does the [psobject] here actually do anything, or is it just a type declaration for our reference?

Just a type declaration to make clear this function accepts any type.

dahlbyk · 2018-01-01T20:59:25Z

src/AnsiUtils.ps1

-            if ($ColorTranslatorType) {
-                $color = $ColorTranslatorType::FromHtml($color)
+            if ($color -as [ConsoleColor]) {
+                $color = [System.ConsoleColor]$color


For my reference, is there an advantage to fully qualifying [ConsoleColor]? We should probably be consistent.

Slight perf improvement. PowerShell looks for [ConsoleColor] in the global namespace first, when that fails, it prepends System. and tries again.

dahlbyk · 2018-01-01T21:20:42Z

src/posh-git.psd1

@@ -4,7 +4,7 @@
 RootModule = 'posh-git.psm1'

 # Version number of this module.
-ModuleVersion = '1.0.0.0'
+ModuleVersion = '1.0.0'


Does this have to change? It seems to have been helpful on bug reports to let the last .0 differentiate between cloned-from-source and actual releases, but I may be imagining that?

I guess what I'm saying is: adjusting this file is part of the release process, but we probably don't want to actually commit it. Thoughts?

It definitely needs to be 1.0.0 when it is published - see https://docs.microsoft.com/en-us/powershell/gallery/psget/module/prereleasemodule.

I worry a bit about precedence order when a user has a prerelease installed and has cloned the repo. But perhaps the fact that our repo dir struct doesn't lend itself to being added to PSModulePath works in our favor in this case.

In trying to test the precedence order with the new SemanticVersion type in PS Core, I see that it chokes on 1.0.0.0:

14:143ms> [System.Management.Automation.SemanticVersion]'1.0.0.0' Cannot convert value "1.0.0.0" to type "System.Management.Automation.SemanticVersion". Error: "Input string was not in a correct format." At line:1 char:1 + [System.Management.Automation.SemanticVersion]'1.0.0.0' + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + CategoryInfo : InvalidArgument: (:) [], RuntimeException + FullyQualifiedErrorId : InvalidCastParseTargetInvocation [rkeithhill/refactor-write-prompt ≡] ~\GitHub\dahlbyk\posh-git 15:96ms> [System.Management.Automation.SemanticVersion]'1.0.0-beta1' Major Minor Patch PreReleas BuildLabel eLabel ----- ----- ----- --------- ---------- 1 0 0 beta1

That still doesn't mean we can't leave it at 1.0.0.0 in the repo but perhaps we should think about moving away from this strategy.

I wish the newer version of PowerShellGet reported the pre-release info. If it did, I'd say we leave it at 1.0.0 and set the Prerelease private data field to alpha0. That would imply we would want to bump the patch to 1.0.1 right after release so that a local/cloned 1.0.1-alpha0 would load ahead of 1.0.0.

The issue template includes PreReleaseVersion, so as long as that's something dev-ish we should be fine. 1.0.0-dev?

Doesn't -dev sort after -alpha and -beta in semver 1? That's why I suggested alpha0 for the checked in version of the psd1 file. Or we could just make that alpha since alpha sorts before alpha0.

I guess my thought was that a local version should always take precedence over a packaged version, but I suppose an actual prerelease of 1.0.1 should take priority over the first commit on the way to 1.0.1? Really not worth overthinking...

But since overthinking is fun, the SemVer 1.0.0 spec says:

A pre-release version number MAY be denoted by appending an arbitrary string immediately following the patch version and a dash. The string MUST be comprised of only alphanumerics plus dash [0-9A-Za-z-]. Pre-release versions satisfy but have a lower precedence than the associated normal version. Precedence SHOULD be determined by lexicographic ASCII sort order. For instance: 1.0.0-alpha1 < 1.0.0-beta1 < 1.0.0-beta2 < 1.0.0-rc1 < 1.0.0.

So, a PreReleaseVersion of -0 seems to have the absolute lowest precedence, but I'm fine with -alpha/-beta (no number) indicating dev versions and actual prereleases of -alpha0, -alpha1, -beta0, etc.

Updated to set it to alpha. According to docs (and peeking at PSReadline beta) the - prefix is not required.

dahlbyk · 2018-01-01T21:22:58Z

src/GitPrompt.ps1

-    BeforeBackgroundColor                       = $null
+class PoshGitCellColor {
+    [psobject]$BackgroundColor
+    [psobject]$ForegroundColor


Since the class is named Color, thoughts on renaming to Background/Foreground to make our code less noisy?

I went back and forth on this one. Even though the class has the name color in it, when it comes to using it in Intellisense (or reading it), what matters is the name of the property that is of type [PoshGitCellColor]. I was also channeling CSS a bit - background-color and foreground-color.

I made this comment before I had reviewed most of the code. We don't refer to these properties by name that much, so let's leave them.

dahlbyk · 2018-01-01T21:28:22Z

src/GitPrompt.ps1

-    BeforeText                                  = ' ['
-    BeforeForegroundColor                       = [ConsoleColor]::Yellow
-    BeforeBackgroundColor                       = $null
+class PoshGitCellColor {


Should these supporting types live in a separate file?

Sure could. I was just following my perception that we tend to group related things (i.e. multiple functions) into a single script file. GitPrompt.ps1 is the only place these classes are used. But I have no problem breaking them out into separate script files. Would we want a single file for all the classes or class per file? BTW I believe there is a small module startup perf hit the more files you dot source.

We do tend to group related things, and I don't have concrete intuition for how big is too big. I guess my gut says including them here is too big, but it's not worth having a file per. Maybe PoshGitTypes.ps1?

Sounds good. I'll make it so.

dahlbyk · 2018-01-01T22:47:31Z

src/GitPrompt.ps1

+        $sb | Write-WorkingDirectoryStatus $Status > $null
+    }
+
+    $sb | Write-WorkingDirectoryLocalStatus $Status > $null


This would be a change in behavior, but it doesn't make sense to call this if EnableFileStatus is off.

dahlbyk · 2018-01-01T22:50:50Z

src/GitPrompt.ps1

+    System.Management.Automation.PSCustomObject
+        This is PSCustomObject returned by Get-GitStatus
+.OUTPUTS
+    PoshGitTextSpan


Should this return a PoshGitCellColor?

This points out a bit of a flaw where [PoshGitTextSpan] allows the user to specify a custom ANSI seq but [PoshGitCellColor] doesn't. That sequence is output before the text span's Text and we automatically close the seq with ${esc}[0m. The use of a custom ANSI seq renders Fg/BgColor unused. So if you want to control both, make sure both are set in the ANSI seq.

[PoshGitCellColor] color doesn't have a $CusomAnsi property and perhaps it should. If so, then yeah, this function should return [PoshGitCellColor]. Without that support, I didn't want to lose any custom ANSI the user may have specified in the various Branch*StatusSymbol properties (which are of type [PoshGitTextSpan]).

dahlbyk · 2018-01-01T22:53:23Z

src/GitPrompt.ps1

@@ -415,146 +575,323 @@ function Write-BranchStatus {

    $str = ""
    if ($branchStatusTextSpan.Text) {
+        $textSpan = [PoshGitTextSpan]::new($branchStatusTextSpan)
+        $textSpan.Text = " " + $branchStatusTextSpan.Text


This still strikes me as a bit awkward. Could we teach Write-Prompt to understand -Prefix ' ' or something?

dahlbyk · 2018-01-01T22:58:07Z

src/posh-git.psd1

+    'Write-GitBranchStatus',
+    'Write-GitIndexStatus',
+    'Write-GitStashCount',
+    'Write-GitWorkingDirStatus',


How about combining Write-GitIndexStatus, Write-GitWorkingDirStatus and Write-GitWorkingDirLocalStatus as Write-GitFileStatus, named in alignment with EnableFileStatus? Then instead of checking the flag outside the function, we'd check it once inside the function and return early if appropriate.

dahlbyk · 2018-01-01T23:01:20Z

src/GitPrompt.ps1

+    [bool]$AnsiConsole = $Host.UI.SupportsVirtualTerminal -or ($Env:ConEmuANSI -eq "ON")
+
+    [PoshGitCellColor]$DefaultColor   = [PoshGitCellColor]::new()
+    [PoshGitCellColor]$BranchColor    = [PoshGitCellColor]::new([ConsoleColor]::Cyan)


Are PowerShell classes able to define type coersion? Would be handy to be able to assign [ConsoleColor]::Cyan here and have it automatically convert to a foreground-only color.

PS Prompt wants a single string - not an array of strings. Address PR feedback on using PoshGitTextSpan in error handler.

dahlbyk · 2018-01-02T02:31:22Z

The more I think on this, the more I'm convinced PoshGitTextSpan needs to get dumber (essentially a tuple of text/color) and PoshGitCellColor needs to get smarter. I feel like we're jumping through a lot of hoops to turn things into the former, when we already have the latter. In particular, it feels like we can simplify quite a bit if we're mostly doing this:

$workingText = " $($s.FileAddedText)$($status.Working.Added.Count)"
$sb | Write-Prompt $s.WorkingColor $workingText > $null

Instead of this:

$workingTextSpan = [PoshGitTextSpan]::new($s.WorkingColor)
$workingTextSpan.Text = " $($s.FileAddedText)$($status.Working.Added.Count)"
$sb | Write-Prompt $workingTextSpan > $null

rkeithhill · 2018-01-02T03:12:00Z

Or we could just add another parameter set to Write-Prompt that takes a [PoshGitCellColor] instead of the Foreground/BackgroundColor parameters, then this would work as-is:

$workingText = " $($s.FileAddedText)$($status.Working.Added.Count)"
$sb | Write-Prompt $workingText $s.WorkingColor > $null

dahlbyk · 2018-01-02T04:53:11Z

src/GitPrompt.ps1

+        }
+    }
+
+    $sb.ToString()


I may be oversimplifying, but isn't this the same as:

$OFS = "" "$($global:VcsPromptStatuses | ForEach-Object { & $_ })"

Probably not. I was following the pattern we'd established in Write-Git*Status methods. Just thinking of the case where AnsiConsole is disabled - that should write to the host and not return anything (or empty strings) and those should concat to an empty string. So yeah, that should work. Want me to change this (and test it)?

Sure, I'm all for less code. 99.99+% of the time there's a single status function with a single result.

dahlbyk · 2018-01-02T05:16:15Z

src/PoshGitTypes.ps1

+
+    [PoshGitTextSpan]$LocalDefaultStatusSymbol = [PoshGitTextSpan]::new('', [ConsoleColor]::DarkGreen)
+    [PoshGitTextSpan]$LocalWorkingStatusSymbol = [PoshGitTextSpan]::new('!', [ConsoleColor]::DarkRed)
+    [PoshGitTextSpan]$LocalStagedStatusSymbol = [PoshGitTextSpan]::new('~', [ConsoleColor]::DarkCyan)


In case you missed the comment on an outdated line: was this deliberately changed from Cyan to DarkCyan?

Looks like that was in the very first commit I made back on 10/28. I honestly don't remember. I wonder if my color scheme was having issues with Cyan that day? In my current scheme (OneHalfDark) I can't see any diff between Cyan and DarkCyan. Want me to change it back to Cyan?

Also redo column alignment that got reset after paste into new file.

This is a weird one. I only found it by debugging the script. While `"Black" -as [ConsoleColor]` returns `System.ConsoleColor.Black`, that doesn't cause the `if` condition to be true. Ah, the issue is that the enum value for Black is 0. Doh!

This cleaned up a number of the Write-Git*Status functions a bit. Fixed bug in Write-GitStashCount where we were incorrectly redirecting the $str assignments to $null.

rkeithhill · 2018-01-03T20:22:16Z

I think I've addressed most (all?) of the concerns so far. So where are we at with this one?

dahlbyk · 2018-01-03T22:48:01Z

I'll rebase this tonight, which should identify any remaining feedback.

rkeithhill added 11 commits October 28, 2017 20:49

WIP: A simpler take at organizing settings using v5 classes

a7b01c5

Reorder some settings

7460daf

Make sure value is not null

98c61bb

Try another approach with custom ansi

b28513c

WIP - refactor Write-Prompt

4d8e08c

WIP refactor Write-GitStatus into individual Write-* methods.

8fcfc21

Not sure whether all these individual methods should be exposed but I'm inclined to expose them so folks can compose their Git status in their own order.

Prefix new function nouns with "Git"

7fb0d6d

Rename GitWorkingDirectory to GitWorkingDir

8dee3e7

Favor ConsoleColor over HtmlColor

71a085a

Change var name from $res to $prompt

fa2b05b

rkeithhill requested a review from dahlbyk December 29, 2017 06:53

rkeithhill commented Dec 31, 2017

View reviewed changes

rkeithhill added 3 commits December 31, 2017 18:04

Add Write-GitBranchName

5120624

Rename Write-GitWorkingDirLocalStatus to Write-GitWorkingDirStatusSummary Add -NoLeadingSpace parameter to individual Write-Git* functions to control output of leading space.

Fix a typo in help comment

88312b2

rkeithhill commented Jan 1, 2018

View reviewed changes

This was referenced Jan 1, 2018

WIP: ANSI and settings prototype #456

Closed

WIP: Reorgs settings using PS v5 classes #499

Closed

Export $GitPromptScriptBlock variable

db734a6

Allow advanced users to invoke the $scriptblock from their own prompt functions.

rkeithhill mentioned this pull request Jan 1, 2018

Update develop from master #517

Merged

Merge from develop

b7b58bc

dahlbyk requested changes Jan 1, 2018

View reviewed changes

rkeithhill added 2 commits January 1, 2018 17:34

Fix Write-VcsStatus issue with returning array of prompt strings

53cb658

PS Prompt wants a single string - not an array of strings. Address PR feedback on using PoshGitTextSpan in error handler.

Address PR feedback on -Status param help

a3a4ffe

Address PR feedback to move types to separate file

2af60e0

dahlbyk reviewed Jan 2, 2018

View reviewed changes

Adjust Prerelease field - set to alpha

6235734

dahlbyk reviewed Jan 2, 2018

View reviewed changes

rkeithhill added 4 commits January 1, 2018 22:17

Address PR feedback - simplify Write-VcsStatus

9f9a649

Address PR feedback - DarkCyan back to Cyan

635c113

Also redo column alignment that got reset after paste into new file.

Add -Color parameter to Write-Prompt

2831c0c

This cleaned up a number of the Write-Git*Status functions a bit. Fixed bug in Write-GitStashCount where we were incorrectly redirecting the $str assignments to $null.

dahlbyk changed the title ~~WIP Refactor Write-GitStatus function~~ Refactor Settings & Write-GitStatus function Jan 10, 2018

dahlbyk approved these changes Jan 10, 2018

View reviewed changes

dahlbyk merged commit 64d2da4 into develop Jan 10, 2018

dahlbyk mentioned this pull request Jan 10, 2018

Default Prompt Colors #520

Merged

This was referenced Jan 10, 2018

Replace or improve $GitPromptSettings #344

Closed

Bust up Write-GitStatus into smaller functions to make it easier for individuals to override parts of the status display #345

Closed

dahlbyk mentioned this pull request Jan 10, 2018

Release v0.7.2 #522

Merged

rkeithhill deleted the rkeithhill/refactor-write-prompt branch May 14, 2018 03:36

Refactor Settings & Write-GitStatus function #513

Refactor Settings & Write-GitStatus function #513

Conversation

rkeithhill commented Dec 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

rkeithhill Jan 2, 2018 • 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

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

rkeithhill Jan 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dahlbyk Jan 1, 2018 • edited Loading

Choose a reason for hiding this comment

dahlbyk commented Jan 2, 2018

rkeithhill commented Jan 2, 2018

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

rkeithhill commented Jan 3, 2018

dahlbyk commented Jan 3, 2018

rkeithhill Jan 2, 2018 •

edited

Loading

rkeithhill Jan 1, 2018 •

edited

Loading

dahlbyk Jan 1, 2018 •

edited

Loading