Adds emphasis to Select-String default formatter #8963

derek-xia · 2019-02-24T00:51:59Z

PR Summary

Adds the -Emphasize parameter for Select-String and makes it work with -AllMatches and -SimpleMatch.
The -Emphasize parameter highlights the matched text using the negative escape sequence. Issue #2905.
Started at HackIllinois.

PR Context

Makes it easier to distinguish what users are looking for when using the Select-String cmdlet. Helps resolve issue #2905.

PR Checklist

msftclas · 2019-02-24T00:52:10Z

All CLA requirements met.

rkeithhill · 2019-02-24T00:58:47Z

I would suggest that you check to see if the host supports virtual terminal sequences, and if so, use a VT sequence instead of asterisks. Using any character is a bit problematic because you may have that character in the text your searching. That could confuse the user.

FWIW, this is how grep "highlights" matched text:

The tricky part is figuring out what color (or effect e.g. reverse) to use for matched text. And whatever you pick, you probably need to make it configurable.

TylerLeonhardt · 2019-02-24T03:13:43Z

Thanks @rkeithhill for the guidance 😊

This is a student at HackIllinois. @rjmholt and I are working with the students here and are reviewing PRs that they send.

All comments are welcome 😊

rkeithhill · 2019-02-24T03:14:46Z

FYI I find this VT sequence resource very handy: https://docs.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences

vexx32

I also think that highlighting with some form of colour is a more worthwhile approach, as the matched string itself could easily contain asterisks or really any number of other characters that we might try to emphasize it with.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/MatchString.cs

derek-xia · 2019-02-24T07:27:39Z

I made the matched text red on terminals that support VT sequences and retained the asterisks on terminals that don't support VT sequences.

TylerLeonhardt · 2019-02-24T07:49:31Z

@derek-xia can you add a screenshot so everyone can see?

derek-xia · 2019-02-24T07:58:52Z

This is a screenshot of -Emphasize with and without -AllMatch

src/Microsoft.PowerShell.Commands.Utility/commands/utility/MatchString.cs

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-String.Tests.ps1

src/Microsoft.PowerShell.Commands.Utility/commands/utility/MatchString.cs

iSazonov · 2019-02-24T17:44:39Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/MatchString.cs

@@ -1518,6 +1600,9 @@ private bool DoMatchWorker(string operandString, MatchInfo matchInfo, out MatchI
            int patternIndex = 0;
            matchResult = null;

+            var indexes = new List<int>();
+            var lengths = new List<int>();


I wonder why we need this if already have the same in matchResult.Matches?
Note that in both cases we have disordered list.

I'm creating new lists in all cases, although I could use matchResult.Matches in most cases and allocate and use these lists only when the SimpleMatch parameter is enabled. If the SimpleMatch parameter is enabled, then the current behavior is that Matches does not contain any Match objects. Without any Match objects, I would need to pass in these lists to implement the colored text when SimpleMatch is enabled.

I think we should avoid creating or populating lists in the (possibly common) case where Emphasize is not specified or where VT100 isn't available. We don't want a performance hit in cases where there's no benefit.

That might look something like:

List<int> indexes; List<int> lengths; if (Emphasize) { indexes = new List<int>(); lengths = new List<int>(); } ... if (Emphasize) { foreach (Match match in matches) { indexes.Add(match.Index); lengths.Add(match.Length); } } // etc. ... matchResult = Emphasize ? new MatchInfo(indexes, lenghts) : new MatchInfo(); matchResult.IgnoreCase = !CaseSensitive; Line = operandString; // etc. ...

iSazonov · 2019-02-24T17:48:46Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-String.Tests.ps1

@@ -71,6 +71,33 @@ Describe "Select-String" -Tags "CI" {
 	    $testinputone | Select-String -Pattern "goodbye" -NotMatch | Should -Be "hello", "hello"
 	}

+	it "Should return an array of matching strings with the first match highlighted when Emphasize is used" {
+		if ($IsWindows)


Doesn't Windows support VT?

Yeah rather than doing $IsWindows you can do:

$Host.UI.SupportsVirtualTerminal

Which returns a bool.

The first version of Windows 10 (1507) did not support VT. Probably not many folks still using that version but $Host.UI.SupportsVirtualTerminal works just as well. And you may be running in a different host than conhost.

But those hosts would just not display the color, right?

Would also be best to consider https://no-color.org/

felixfbecker · 2019-02-24T19:50:42Z

Does it highlight capture group like in #2905?

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-String.Tests.ps1

vexx32 · 2019-02-24T22:47:34Z

@joeyaiello mentioned on Twitter that might be worth considering just making this behaviour the default. Perhaps that might be worth doing? If there are concerns with stability / how different conhosts would work with this we can always stick it behind an experimental feature flag. @TylerLeonhardt, what do you think?

TylerLeonhardt · 2019-02-24T23:39:46Z

@vexx32, I think @felixfbecker brought up a very good point - this is more of a formatting concern.

Looking at the formatting code, it seems that formatting MatchInfo to be printed to the console is done here. Where it simply runs:

$_.ToString(((get-location).path))

where $_ is the instance of MatchInfo

All it's doing is calling the ToString. Perhaps what would be better is move @derek-xia's logic added to ToString to a new public method on MatchInfo called Emphasize and then the formatting line linked above could just call:

$_.Emphasize(((get-location).path))

Does that sound alright @vexx32 @felixfbecker?

rkeithhill · 2019-02-25T00:40:39Z

Or maybe call it ToAnsiString().

iSazonov · 2019-02-25T05:06:58Z

Considering that we would like to receive color input for any output type, this should not be in every single cmdlet, it should be out.

powercode · 2019-02-25T10:33:48Z

I don't think this change should not be made in Select-String.

You can check out this gist that adds coloring of the matchinfo output as a custom format. https://gist.github.com/powercode/4833804efd23045387bd5d5249d76f7b

There already exists an issue for improving the formatting of MatchInfo, but this is a bit of "*nix" thinking where flags on a command modify the output. Make it a view in the formatting system instead.

rjmholt · 2019-02-25T18:47:40Z

I feel like we're all agreeing here. Just want to summarise:

This behaviour should be in the formatter
Most efficient implementation is probably as @rkeithhill suggests with a ToAnsiString() method on MatchInfo objects. Possibly ToEmphasizedString() if we want that to return something still useful on a platform that does not support ANSI/VT control codes (like emphasis characters around the match).
Given that it would be in the formatter, we could have it on by default (can changing formatting be a breaking change?)

My only question is: should there be a switch for this on the cmdlet?

vexx32 · 2019-02-25T19:03:17Z

If I recall correctly, changes to formatting are not considered "breaking."

Emphasis characters are kind of hard to argue for, really. There's no guarantee such characters won't already be somewhere in or around the matched string, unfortunately. Might still be worth doing, I suppose, but honestly I feel simply omitting the formatting where it's not supported is likely to be most effective.

I don't think there need be a switch for it. Maybe define one formatview for emphasized and one for non-emphasized?

iSazonov · 2019-02-25T19:55:33Z

I like the coloring but I am not a fan of turning PowerShell into an artist. We do not embed a code formatter, but simply publish AST API. The same with coloring. Perhaps it is not formatting system api but conhost api.
Turn on/off:

$PSColoring = $true # Globally
Get-Date | Format-List -Coloring # Locally

rjmholt · 2019-02-25T19:58:36Z

@iSazonov I agree with you in principle, but I think this kind of match emphasis brings PowerShell up to speed with grep in stakes that PowerShell is usually better at: interactive experience.

If we can get this into the formatter, then we can remove or refactor it later with no obligation not to break things.

So making the suggested change in the immediate gets us what we want, without committing us to anything down the road (except a public API on MatchInfo that we shouldn't guarantee dependable behaviour on).

The kind of change you're talking about will require committee approval and would be a whole heap of work. It's wanted, but we can have something small in the meantime too.

rjmholt · 2019-02-25T20:00:01Z

$PSColoring = $true # Globally
Get-Date | Format-List -Coloring # Locally

Maybe worth writing up an RFC for this?

iSazonov · 2019-02-25T20:07:11Z

we can have something small in the meantime too.

We already have the "small" - it is Jason's PSMore. Implementing vNext formatting system based on classes simplify adding new feature such as coloring (new interface?). So I think right way is implementing PSMore project first. (Before Coloring RFC)

felixfbecker · 2019-02-25T21:08:07Z

Advanced output formatting is and has always been part of PowerShell - if your shell is object based, and you also want to provide a strong interactive experience, you need a good formatting system, that supports tables, nested data, grouping, truncation, and coloring too. But all of this is separate from the data and API. It shouldn't be exposed in a parameter switch, nor in the returned strings.

I like the idea of respecting the user's choice regarding not wanting color output. However I feel like PowerShell has the opportunity here to handle that on a shell level? E.g. it could define a well-known global variable $PSEnableColors that if set to $false, will make PowerShell trim all color escape sequences from formatting output, so that not each cmdlet has to deal with it on their own.

TylerLeonhardt · 2019-02-25T21:30:01Z

@iSazonov... Do you mean https://github.com/lzybkr/PSMore

Not sure what the status is on that project... (cc @lzybkr)

I want to also remind everyone here that some folks that submit PRs do not know everything about PowerShell and we should treat that as a learning experience to teach folks about PowerShell-specific concepts.

powercode · 2019-02-25T21:43:13Z

I think the jury is still out on how do handle colors. I have started on an implementation for better coloring of select-string, but deferred it until we know what to do with colors. That opens up a can of worms, with theming etc. (which may be very important for the color-blind).

rjmholt

LGTM

SteveL-MSFT · 2019-08-29T22:42:34Z

Looks like the last issue is that the test needs to be fixed

TylerLeonhardt · 2019-09-25T16:06:10Z

there was a missing brace so I quickly added it :) lets see what CI says but this should be good to go.

SteveL-MSFT · 2019-09-28T17:33:23Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-String.Tests.ps1

@@ -127,6 +127,7 @@ Describe "Select-String" -Tags "CI" {

 	It "Should return ParameterBindingException when -Raw and -Quiet are used together" {
 		{ $testinputone | Select-String -Pattern "hello" -Raw -Quiet -ErrorAction Stop } | Should -Throw -ExceptionType ([System.Management.Automation.ParameterBindingException])
+        }


This brace seems out of place? I think this whole file should be reformatted so the intention is correct making it easier to understand the braces.

Maybe that would be better done in a quick follow-up PR so we can keep track of the diffs and so accidental changes are made?

The reformat can be a single commit. Then the fix of the brace a separate commit.

That test isn't touched in this PR. My last commit just undid an accidental deletion of a brace.

@TylerLeonhardt perhaps you can just add a commit after reformatting the tests?

What's wrong with the formatting exactly? I don't see anything wrong with the changes in this PR.

Not this PR specifically, but if you view the whole file, the indentation is all over the place. Makes it hard to review the code. I still can't tell how that missing brace fixed the code since the indentation isn't consistent.

The indentation issue is because about half the file is indented with tabs instead of spaces. Github renders a tab as 8 spaces, whereas most editors commonly only render it with 4 spaces (though this is configurable).

I think VS Code has a convert tabs to spaces option in its command palette to get rid of the tabs, and then you can do a quick skim over the file to fix any remaining issues.

Ok, I see. For this repo, we've standardized on spaces.

TylerLeonhardt · 2019-09-30T20:44:07Z

@SteveL-MSFT please rereview

SteveL-MSFT

@TylerLeonhardt I realize my feedback is in the existing code and not part of this specific PR. So if you want to fix it as part of this PR, that's fine, or if you want to submit a new PR that's fine too.

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-String.Tests.ps1

iSazonov · 2019-10-01T03:48:10Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/MatchString.cs

+        /// Surrounds the matched text with virtual terminal sequences to invert it's color. Used in ToEmphasizedString.
+        /// </summary>
+        /// <returns>The matched line with matched text inverted.</returns>
+        private string EmphasizeLine()


Good candidate to be static.

But it uses _matchIndexes? Or do you mean refactor it to be static that takes in _matchIndexes and _matchLengths

We can pass _matchIndexes by parameter.

I agree. Just like a whole bunch of other PowerShell code that is similarly caught up by mutated object state implicitly passed in to methods.

But I believe that pre-existed here and this contribution shouldn't be held back on the basis of code debt incurred before PowerShell was open-sourced. This PR has been open long enough and has been a good, responsive, constrained contribution and I think we really need to just accept or reject it, rather than turning it into a Ship of Theseus.

Yeah, I think this one's gone on long enough and is a definite nice-to-have in PSv7 😁

We can always make other smaller changes later. 🙂

The PR is delayed because it is in new area for PowerShell. However, the code should be clean.

Since this isn't a public API, we can always refactor internal code. I would suggest a separate PR if it's a concern or @iSazonov you can always add a commit to this PR if you feel strongly about this one.

Yes, I can :-) Do you ok that we have over 70 opened PR? :-)

@iSazonov I'll ask the team to start driving down the number of open PRs

adityapatwardhan · 2019-10-08T23:11:25Z

@derek-xia Thank you for your contribution!

TylerLeonhardt · 2019-10-09T00:40:28Z

AT LAST! Well done @derek-xia 🎉

ghost · 2019-10-23T19:08:09Z

🎉v7.0.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

TylerLeonhardt · 2019-11-28T10:36:04Z

I'm just gonna leave this here...

https://twitter.com/fatherjack/status/1199807445037723648?s=19

😁

Twitter
FatherJack 🏁 on Twitter
“Can everyone upgrade to #PowerShell 7 right away please? I have seen the future and it is select-string with match highlighting.”

derek-xia requested review from JamesWTruher and PaulHigin as code owners February 24, 2019 00:51

vexx32 reviewed Feb 24, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/MatchString.cs Outdated Show resolved Hide resolved

rjmholt reviewed Feb 24, 2019

View reviewed changes

iSazonov reviewed Feb 24, 2019

View reviewed changes

felixfbecker reviewed Feb 24, 2019

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-String.Tests.ps1 Outdated Show resolved Hide resolved

felixfbecker reviewed Feb 24, 2019

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-String.Tests.ps1 Outdated Show resolved Hide resolved

rjmholt added the Hacktoberfest Potential candidate to participate in Hacktoberfest label Feb 25, 2019

derek-xia added 3 commits August 27, 2019 14:13

shouldEmphasize

f840d88

Formatting changes

9395045

Changed Emphasize to NoEmphasis

f9145bd

derek-xia force-pushed the Select-String-Color branch from 9e6b1e1 to f9145bd Compare August 27, 2019 19:33

rjmholt approved these changes Aug 28, 2019

View reviewed changes

TylerLeonhardt added 2 commits September 25, 2019 09:03

add right }

ccc09f8

tab instead of spaces?

46e62fd

SteveL-MSFT requested changes Sep 28, 2019

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 28, 2019

tabs to spaces

40facf5

SteveL-MSFT approved these changes Sep 30, 2019

View reviewed changes

address steve's comments

52a9c0f

iSazonov reviewed Oct 1, 2019

View reviewed changes

SteveL-MSFT removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 1, 2019

TylerLeonhardt assigned adityapatwardhan and unassigned TravisEz13 Oct 1, 2019

adityapatwardhan approved these changes Oct 8, 2019

View reviewed changes

adityapatwardhan merged commit 4a4dc4c into PowerShell:master Oct 8, 2019

adityapatwardhan added this to the 7.0.0-preview.5 milestone Oct 8, 2019

KirkMunro mentioned this pull request Oct 9, 2019

Make Select-String more intelligent for non-primitive types #10726

Closed

TylerLeonhardt mentioned this pull request Oct 12, 2019

Update upgrade notification message #10777

Merged

14 tasks

kilasuit pushed a commit to kilasuit/PowerShell that referenced this pull request Oct 31, 2019

Adds emphasis to Select-String default formatter (PowerShell#8963)

236ffb6

kilasuit pushed a commit to kilasuit/PowerShell that referenced this pull request Nov 9, 2019

Adds emphasis to Select-String default formatter (PowerShell#8963)

8e12b00

Adds emphasis to Select-String default formatter #8963

Adds emphasis to Select-String default formatter #8963

Conversation

derek-xia commented Feb 24, 2019 • edited by TravisEz13 Loading

PR Summary

PR Context

PR Checklist

msftclas commented Feb 24, 2019 • edited Loading

rkeithhill commented Feb 24, 2019 • edited Loading

TylerLeonhardt commented Feb 24, 2019

rkeithhill commented Feb 24, 2019

vexx32 left a comment

Choose a reason for hiding this comment

derek-xia commented Feb 24, 2019

TylerLeonhardt commented Feb 24, 2019

derek-xia commented Feb 24, 2019 • 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

rkeithhill Feb 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker commented Feb 24, 2019

vexx32 commented Feb 24, 2019

TylerLeonhardt commented Feb 24, 2019

rkeithhill commented Feb 25, 2019

iSazonov commented Feb 25, 2019

powercode commented Feb 25, 2019

rjmholt commented Feb 25, 2019

vexx32 commented Feb 25, 2019

iSazonov commented Feb 25, 2019

rjmholt commented Feb 25, 2019 • edited Loading

rjmholt commented Feb 25, 2019

iSazonov commented Feb 25, 2019 • edited Loading

felixfbecker commented Feb 25, 2019

TylerLeonhardt commented Feb 25, 2019

powercode commented Feb 25, 2019

rjmholt left a comment

Choose a reason for hiding this comment

SteveL-MSFT commented Aug 29, 2019

TylerLeonhardt commented Sep 25, 2019

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

TylerLeonhardt commented Sep 30, 2019

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

vexx32 Oct 1, 2019 • 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

adityapatwardhan commented Oct 8, 2019

TylerLeonhardt commented Oct 9, 2019

ghost commented Oct 23, 2019

TylerLeonhardt commented Nov 28, 2019 • edited by unfurl-links bot Loading

derek-xia commented Feb 24, 2019 •

edited by TravisEz13

Loading

msftclas commented Feb 24, 2019 •

edited

Loading

rkeithhill commented Feb 24, 2019 •

edited

Loading

derek-xia commented Feb 24, 2019 •

edited

Loading

rkeithhill Feb 24, 2019 •

edited

Loading

rjmholt commented Feb 25, 2019 •

edited

Loading

iSazonov commented Feb 25, 2019 •

edited

Loading

TylerLeonhardt Oct 1, 2019 •

edited

Loading

rjmholt Oct 1, 2019 •

edited

Loading

vexx32 Oct 1, 2019 •

edited

Loading

TylerLeonhardt commented Nov 28, 2019 •

edited by unfurl-links bot

Loading