Skip to content
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

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

Closed
wants to merge 4 commits into from

Conversation

nanyiyang
Copy link

@nanyiyang nanyiyang commented Feb 24, 2019

PR Summary

Part of HackIllinois with @TylerLeonhardt and @rjmholt
Added an OnlyMatching parameter to Select-String with similar behavior to grep -o. Also made OnlyMatching parameter to work with AllMatches.

PR Context

Resolves #7712.

PR Checklist

@msftclas
Copy link

msftclas commented Feb 24, 2019

CLA assistant check
All CLA requirements met.

Copy link
Collaborator

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of small things I think should probably be fixed, but otherwise solid work from what I can see. 👌

@@ -1048,6 +1048,12 @@ public string[] LiteralPath
[Parameter]
public SwitchParameter List { get; set; }

/// <summary>
/// Gets or sets a value indicating if the search contains the specified characters.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit unclear, as Select-String already returns data containing the specified characters. Does this parameter affect the search or the result that the user sees or receives?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the parameter should be affecting the result that the user sees. I will update the comment and re-commit.

vexx32 and others added 3 commits February 24, 2019 00:33
…chString.cs

Co-Authored-By: nanyiyang <nanyiry2@illinois.edu>
…chString.cs

Co-Authored-By: nanyiyang <nanyiry2@illinois.edu>
@@ -1048,6 +1048,12 @@ public string[] LiteralPath
[Parameter]
public SwitchParameter List { get; set; }

/// <summary>
/// Gets or sets a value that when true, causes only the matching part of the string to be returned.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting that this causes a string rather than a MatchInfo to be returned

@TylerLeonhardt
Copy link
Member

How does this change work with -AllMatches?

I'd probably expect something like this:

PS > "asdf 123 asdf" | Select-String \d -OnlyMatching -AllMatches
1
2
3

@powercode
Copy link
Collaborator

Is this really a change we want to make?
The same result can be accomplished on the command line with | % Line, right?

The *nix commands has lots of flags to change the output format of commands, but this is generally not needed in PowerShell since we have structured objects in the pipeline.

Tab completion will have to be fixed, since this parameter changes the output type.

Can you help me with a scenario where this is change is compelling?

@rjmholt rjmholt added the Hacktoberfest Potential candidate to participate in Hacktoberfest label Feb 25, 2019
@TylerLeonhardt
Copy link
Member

I want to ping @mklement0 to get his thoughts but @powercode I don't think that accomplishes the same goal.

Line gives you the whole line. We want to only get the matching parts of the line.

@powercode
Copy link
Collaborator

Update-TypeData -Typename Microsoft.PowerShell.Commands.MatchInfo -MemberName MatchingText`
  -MemberType ScriptProperty -Value {foreach($m in $this.Matches){foreach($g in $m.Groups){$g.Value}}}

Then

sls … | foreach MatchingText

@mklement0
Copy link
Contributor

@powercode:

The same result can be accomplished on the command line with | % Line, right?

Supporting returning whole lines directly is the subject of #7713 - and both it and the change at hand (matching parts only, via #7712) have been green-lit.

The motivation is the same in both cases:

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Feb 26, 2019

@powercode I can get behind that. We can add another property to MatchInfo called MatchingText and then do:

Select-String "foo" | foreach MatchingText

like you said to get this behavior.

For context to @nanyiyang...

Select-String "foo"

returns a collection of MatchInfo, right? By adding a property called MatchingText to that we can use the ForEach-Object or foreach or % command:

Select-String "foo" | foreach MatchingText

which says, give me just the MatchingText in each of these objects. This would achieve what we want to achieve.

@mklement0
Copy link
Contributor

mklement0 commented Feb 26, 2019

@TylerLeonhardt:

That is only slightly less verbose than what you have to do currently:

# Works with -AllMatches too
PS> 'afoobar' | Select-String 'foo' | foreach { $_.Matches.Value }
foo

and wouldn't yield the hoped-for performance improvements - though I suppose it could be implemented as well (though I personally think -OnlyMatching is sufficient).

Also note that .MatchingText, if implemented as a property of [Microsoft.PowerShell.Commands.MatchInfo], would have to be an array to accommodate the
-AllMatches case.

@TylerLeonhardt
Copy link
Member

Good catch... I feel like in both #7713 and #7712 you're really asking for a different object to be returned by Select-String.

One that doesn't contain the extra fluff for an increase in performance?

@mklement0
Copy link
Contributor

Specifically, I'm asking for strings to be returned - which is arguably Select-Strings's core mandate.

Not having fluff, i.e. not having to access properties in a subsequent step when all you're looking for is strings not only improves performance, but is also concise and convenient.

@mklement0
Copy link
Contributor

I've summed up the rationale for this new switch as well as for -Raw in #8962 (comment); given that this new switch was approved in #7712, I suggest you voice any objections there.

@stale
Copy link

stale bot commented Apr 10, 2019

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 stale bot added the Stale label Apr 10, 2019
@stale
Copy link

stale bot commented Apr 20, 2019

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.

@stale stale bot closed this Apr 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest Potential candidate to participate in Hacktoberfest Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a switch to Select-String that returns the matching parts only, analogous to grep -o
7 participants