[feature] adding select-string -raw #8962

david-wang24 · 2019-02-24T00:06:06Z

PR Summary

Added -Raw switch to select-string for convenience. Issue #7713.

PR Context

Resolves #7713.

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
User-facing changes
- [] Not Applicable
- OR
- Documentation needed
  - [] Issue filed:
Testing - New and feature
- N/A or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed
  - Add [feature] to your commit messages if the change is significant or affects feature tests

msftclas · 2019-02-24T00:06:17Z

All CLA requirements met.

rjmholt

This is looking good -- have left some comments about changes to make

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

rjmholt · 2019-02-24T00:29:28Z

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

@@ -93,6 +107,10 @@ public class MatchInfo
        /// <value>The text of the matching line.</value>
        public string Line { get; set; } = string.Empty;

+


Please remove these newlines

rjmholt · 2019-02-24T00:29:38Z

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

@@ -101,6 +119,9 @@ public class MatchInfo
        /// </remarks>
        /// </summary>
        /// <value>The file name.</value>
+


Please remove these newlines

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

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

TylerLeonhardt · 2019-02-24T03:11:54Z

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

…chString.cs Co-Authored-By: david-wang24 <42791457+david-wang24@users.noreply.github.com>

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

…chString.cs Co-Authored-By: david-wang24 <42791457+david-wang24@users.noreply.github.com>

TylerLeonhardt

Lefts a few comments! This is coming along 😄

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

TylerLeonhardt · 2019-02-24T23:43:07Z

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

+        /// </summary>
+        internal MatchInfo(bool useRaw, bool useColor, int matchIndex, int matchLength)
+        {
+            isRaw = useRaw;


Suggested change

isRaw = useRaw;

_isRaw = useRaw;

TylerLeonhardt · 2019-02-24T23:43:21Z

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

@@ -226,7 +242,15 @@ public string ToString(string directory)
            // enable context-tracking.
            if (Context == null)
            {
-                return FormatLine(Line, this.LineNumber, displayPath, EmptyPrefix);
+                if(isRaw)


Suggested change

if(isRaw)

if(_isRaw)

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

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

@@ -235,14 +259,37 @@ public string ToString(string directory)
            int displayLineNumber = this.LineNumber - Context.DisplayPreContext.Length;
            foreach (string contextLine in Context.DisplayPreContext)
            {
-                lines.Add(FormatLine(contextLine, displayLineNumber++, displayPath, ContextPrefix));
+
+                if(isRaw)


Suggested change

if(isRaw)

if(_isRaw)

TylerLeonhardt · 2019-02-24T23:43:55Z

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

            }

-            lines.Add(FormatLine(Line, displayLineNumber++, displayPath, MatchPrefix));
-
+            if(isRaw)


Suggested change

if(isRaw)

if(_isRaw)

TylerLeonhardt · 2019-02-24T23:44:21Z

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

            foreach (string contextLine in Context.DisplayPostContext)
            {
-                lines.Add(FormatLine(contextLine, displayLineNumber++, displayPath, ContextPrefix));
+                if(isRaw)


Suggested change

if(isRaw)

if(_isRaw)

…Select-String-Convenience [feature] formatting fixes

TylerLeonhardt · 2019-03-07T17:28:26Z

Sorry for the delay, @david-wang24! This is great work! I do have a thought about the implementation.

As we know, in PowerShell, we typically pass around objects everywhere. Into functions, into the pipeline, into variables, everywhere!

What Select-String does today is it returns objects as well of type MatchInfo. But you already knew that 😁

The addition of a -Raw flag in my mind (and @mklement0 who originally opened the issue) is that -Raw would tell Select-String to actually return strings instead of MatchInfos.

What your current implementation does is still returning MatchInfo objects but it's changing the behavior of MatchInfo.ToString() to look like it's only returning the matching lines.

It's still really impressive what you've done so far, but we should look at the problem from a different angle.

Does that make sense? Let me know if you're interested in still working on this 😊no rush

powercode · 2019-03-09T19:45:48Z

I think we should resist the temptation to add nifty switches. The same thing can be accomplished with

… | % Line

vexx32 · 2019-03-09T19:57:05Z

@powercode per the discussion in this PR, that would return the complete matching lines, not just the matched strings from each MatchInfo object.

mklement0 · 2019-03-10T02:08:27Z

Actually, @vexx32, -Raw is indeed about returning full lines, whereas it is -OnlyMatching (#8965) is about only returning what was matched.

@powercode, adding the -Raw switch was greenlit in #7713, and -OnlyMatching was green-lit in #7712. If you feel these features shouldn't have been approved, please voice your concerns there.

To recap the rationale:

Select-String's core mandate - as the name implies - is to return strings. As such, this is what it should always have done by default. Lest we break backward compatibility, the best we can do now is offer the typical use case as an opt-in.
- In hindsight, Select-String could have been patterned after Get-Content, which outputs strings, as expected, that are decorated with NoteProperty members to provide metadata.
- The -Raw switch would still make sense then, however, as an opt-out of the costly act of decoration, as has been proposed for Get-Content in Get-Content is slow on large text files. Could it have a parameter to speed it up by not adding NoteProperties? #7537.
- The case for -Raw as a general opt-out-of-decoration-or-wrapping-in-helper-types was made in Introduce a common parameter pattern for requesting undecorated / not-wrapped-in-helper-types output objects #7855.
| % Line is not only more cumbersome, but performs poorly.

stale · 2019-04-10T05:17:33Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

stale · 2019-04-20T05:18:50Z

This PR has been automatically closed because it is stale. If you wish to continue working on the PR, please first update the PR, then reopen it.
Thanks again for your contribution.
Community members are welcome to grab these works.

[feature] adding select-string-raw

1636c5f

david-wang24 requested review from JamesWTruher and PaulHigin as code owners February 24, 2019 00:06

rjmholt suggested changes Feb 24, 2019

View reviewed changes

david-wang24 and others added 4 commits February 24, 2019 00:49

[feature] fixing rjmholt's comments

0133f5f

Update src/Microsoft.PowerShell.Commands.Utility/commands/utility/Mat…

75fcd91

…chString.cs Co-Authored-By: david-wang24 <42791457+david-wang24@users.noreply.github.com>

[feature] fixing rjmholt's comments

0393b97

[feature] final fixes

94bddad

rjmholt reviewed Feb 24, 2019

View reviewed changes

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

Update src/Microsoft.PowerShell.Commands.Utility/commands/utility/Mat…

c2b0b67

…chString.cs Co-Authored-By: david-wang24 <42791457+david-wang24@users.noreply.github.com>

TylerLeonhardt reviewed Feb 24, 2019

View reviewed changes

david-wang24 added 3 commits February 24, 2019 21:57

[feature] code formatting fixes

6b00448

Merge remote-tracking branch 'origin/Select-String-Convenience' into …

a6d1f2b

…Select-String-Convenience [feature] formatting fixes

[feature] formatting fixes

3b728b2

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

mklement0 mentioned this pull request Mar 11, 2019

Adds an OnlyMatching parameter to Select-String to return only the string that matches the pattern #8965

Closed

11 tasks

stale bot added the Stale label Apr 10, 2019

stale bot closed this Apr 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] adding select-string -raw #8962

[feature] adding select-string -raw #8962

david-wang24 commented Feb 24, 2019 •

edited by rjmholt

msftclas commented Feb 24, 2019 •

edited

rjmholt left a comment

rjmholt Feb 24, 2019

rjmholt Feb 24, 2019

TylerLeonhardt commented Feb 24, 2019

TylerLeonhardt left a comment

TylerLeonhardt Feb 24, 2019

TylerLeonhardt Feb 24, 2019

TylerLeonhardt Feb 24, 2019

TylerLeonhardt Feb 24, 2019

TylerLeonhardt Feb 24, 2019

TylerLeonhardt commented Mar 7, 2019

powercode commented Mar 9, 2019 •

edited

vexx32 commented Mar 9, 2019

mklement0 commented Mar 10, 2019 •

edited

stale bot commented Apr 10, 2019

stale bot commented Apr 20, 2019

		@@ -93,6 +107,10 @@ public class MatchInfo
		/// <value>The text of the matching line.</value>
		public string Line { get; set; } = string.Empty;

[feature] adding select-string -raw #8962

[feature] adding select-string -raw #8962

Conversation

david-wang24 commented Feb 24, 2019 • edited by rjmholt

PR Summary

PR Context

PR Checklist

msftclas commented Feb 24, 2019 • edited

rjmholt left a comment

Choose a reason for hiding this comment

rjmholt Feb 24, 2019

Choose a reason for hiding this comment

rjmholt Feb 24, 2019

Choose a reason for hiding this comment

TylerLeonhardt commented Feb 24, 2019

TylerLeonhardt left a comment

Choose a reason for hiding this comment

TylerLeonhardt Feb 24, 2019

Choose a reason for hiding this comment

TylerLeonhardt Feb 24, 2019

Choose a reason for hiding this comment

TylerLeonhardt Feb 24, 2019

Choose a reason for hiding this comment

TylerLeonhardt Feb 24, 2019

Choose a reason for hiding this comment

TylerLeonhardt Feb 24, 2019

Choose a reason for hiding this comment

TylerLeonhardt commented Mar 7, 2019

powercode commented Mar 9, 2019 • edited

vexx32 commented Mar 9, 2019

mklement0 commented Mar 10, 2019 • edited

stale bot commented Apr 10, 2019

stale bot commented Apr 20, 2019

david-wang24 commented Feb 24, 2019 •

edited by rjmholt

msftclas commented Feb 24, 2019 •

edited

powercode commented Mar 9, 2019 •

edited

mklement0 commented Mar 10, 2019 •

edited