-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Adds emphasis to Select-String default formatter #8963
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
Merged
adityapatwardhan
merged 24 commits into
PowerShell:master
from
derek-xia:Select-String-Color
Oct 8, 2019
Merged
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
ded4f05
[feature] Added Select-String -Color and made it work with -AllMatch …
64534b9
[feature] Added Select-String -Color and test cases
7b48805
[feature] Adds -Emphasize for Select-String
4732734
[feature] Adds red highlighting for matched text
f72f38e
[feature] Adds red highlighting for matched text
00e7224
Apply suggestions from code review
rjmholt ca39e17
[feature] Refactored code
665bd4f
[feature] Moved new functionality, changed formatter, fixed tests
439400d
[feature] Fixed style errors
92c92bc
[feature] Changed red escape sequence to negative escape sequence
6b97206
[feature] Removed surrounding asterisks if VT is unsupported
d299906
[feature] Changed MatchInfo's Emphasize to private
2065727
[feature] Cleaned up matchInfo and DoMatchWorker
derek-xia 6f47e79
[feature] Changed ToEmphasizedString string creation
derek-xia 1bc3437
[feature] Refactored ToEmphasizedString
derek-xia 7623340
Update Select-String.Tests.ps1
TravisEz13 1d23b5c
readd curly brace
TylerLeonhardt f840d88
shouldEmphasize
derek-xia 9395045
Formatting changes
derek-xia f9145bd
Changed Emphasize to NoEmphasis
derek-xia ccc09f8
add right }
TylerLeonhardt 46e62fd
tab instead of spaces?
TylerLeonhardt 40facf5
tabs to spaces
TylerLeonhardt 52a9c0f
address steve's comments
TylerLeonhardt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Good candidate to be static.
Uh oh!
There was an error while loading. Please reload this page.
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.
But it uses
_matchIndexes
? Or do you mean refactor it to be static that takes in_matchIndexes
and_matchLengths
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.
We can pass
_matchIndexes
by parameter.Uh oh!
There was an error while loading. Please reload this page.
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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, 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. 🙂
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.
The PR is delayed because it is in new area for PowerShell. However, the code should be clean.
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.
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.
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.
Yes, I can :-) Do you ok that we have over 70 opened 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.
@iSazonov I'll ask the team to start driving down the number of open PRs