-
-
Notifications
You must be signed in to change notification settings - Fork 809
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
Fixes two bugs - one ANSI and one PS Core #532
Conversation
The ANSI bug is that if both fg and bg are default ($null) we were still emitting the terminating seq `e[0m. The PS Core bug is that on PS Core there is no byte value for Get-Content -Encoding. The replacement is Get-Content -AsByteStream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes look good, but it seems like we could simplify the ANSI code?
src/PoshGitTypes.ps1
Outdated
@@ -101,6 +131,32 @@ class PoshGitTextSpan { | |||
$this.CustomAnsi = $null | |||
} | |||
|
|||
[string] ToEscapedString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this whole method equivalent to EscapeAnsiString $this.RenderAnsi()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Done.
@@ -44,11 +44,41 @@ class PoshGitCellColor { | |||
return $str | |||
} | |||
|
|||
[string] ToEscapedString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense for PoshGitCellColor
to have a RenderAnsi()
, such that this function is essentially EscapeAnsiString $this.RenderAnsi()
? That function could then be used in PoshGitTextSpan
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, would ToAnsiString()
be a better name than RenderAnsi()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I get for coding and watching the playoffs. :-) Renamed RenderAnsi()
to ToAnsiString()
. Can we do the [PoshGitCellColor]
tweaks - ToAnsiString() and ToEscapedString() simplification in another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that works. While you're at it, fix the typo in EscapseAnsiString
. 😏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh! Yeah, will do.
} | ||
|
||
[string] ToEscapedString() { | ||
if ($global:GitPromptSettings.AnsiConsole) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, but should ToEscapedString()
care if the console supports ANSI? Properly encoding/escaping a string doesn't require it…
The ANSI bug is that if both fg and bg are default ($null) we were still emitting the terminating seq `e[0m.
The PS Core bug is that on PS Core there is no byte value for Get-Content -Encoding. The replacement is Get-Content -AsByteStream.
Also. added ToEscapedString() method to [PoshGitCellColor] and [PoshGitTextSpan]. Was invaluable for working on the Pester tests and I think it will be very handy for debugging. If someone's GitPrompSettings isn't displaying properly we can ask them to execute:
$GitPromptSettings.DelimText.ToEscapedString()
This will output:
`e[93m |`e[0m