Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upSupport negative numbers in -split operator #8960
Conversation
ece-jacob-scott
requested review from
BrucePay and
daxian-dbw
as
code owners
Feb 23, 2019
This comment has been minimized.
This comment has been minimized.
msftclas
commented
Feb 23, 2019
•
|
This looks great! I only had some minor whitespace concerns, but besides that, this looks good. |
TylerLeonhardt
changed the title
Enhanced functionality connected to issue 4765
Support negative numbers in -split operator
Feb 23, 2019
This comment has been minimized.
This comment has been minimized.
|
@ece-jacob-scott I forgot to mention that you need to add
|
|
I think Codeacy can be ignored here -- the limit normalisation is nicer. |
This comment has been minimized.
This comment has been minimized.
|
Please consider support for left-to-right splitting in predicate mode as well, ie.: PS C:\> 'a b c d' -split {$_ -like ' '},-2
a b c
d |
|
LGTM |
This comment has been minimized.
This comment has been minimized.
|
Please open new Issue in PowerShell-Docs repo and add reference in the PR description. |
| $res.count | Should -Be 4 | ||
| $res[0] | Should -Be "a" | ||
| $res[1] | Should -Be "b" | ||
| $res[2] | Should -Be "c" | ||
| $res[3] | Should -Be "d" | ||
|
|
||
| $res = "a b c d" -split " ",-8 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ece-jacob-scott
Feb 24, 2019
Author
Contributor
Agreed this is something that I noticed today while showing off my changes to the judges. I will probably come back and add another commit fixing this functionality.
Which way do you think is better? Personally I think that the first item should be at array index 0.
ie
> $r = 'a b c d' -split ' ',-2
> $r[0]
d
This comment has been minimized.
This comment has been minimized.
ece-jacob-scott
Feb 24, 2019
Author
Contributor
Any formatting or implementation details would be greatly appreciated. Thank you! :)
This comment has been minimized.
This comment has been minimized.
rjmholt
Feb 24, 2019
Member
Note that splitting from right to left might be considered orthogonal to the iteration from left to right.
@mklement0's original example in #4765 is:
PS> 'a b c d' -split ' ', -2 # split into (at most) 2 strings from the end
a b c # prefix
d # requested token
PS> 'a b c d' -split ' ', -3 # split into (at most) 3 strings from the end
a b # prefix
c
d
PS> 'a b' -split ' ', -2 # 2 resulting strings - complete split; same as 0 in this case
a
b
This implementation currently accords with that.
This comment has been minimized.
This comment has been minimized.
rjmholt
Feb 24, 2019
Member
Also worth noting that suggestion was approved by the @PowerShell/powershell-committee
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Please look optimizations from https://github.com/PowerShell/PowerShell/pull/5270/files |
rjmholt
added
Review - Committee
Hackathon
labels
Feb 24, 2019
| @@ -651,74 +651,104 @@ private static object SplitOperatorImpl(ExecutionContext context, IScriptExtent | |||
| } | |||
| } | |||
|
|
|||
| private static string ReverseStringUtility(string str) | |||
This comment has been minimized.
This comment has been minimized.
iSazonov
Feb 27, 2019
Collaborator
Can we avoid the reverse?
If no please refactor the method with String.Create() to reduce allocations.
This comment has been minimized.
This comment has been minimized.
IISResetMe
Feb 27, 2019
•
Contributor
Maybe consider iterating over the input string from right-to-left and find all predicate matches in one go, then use string.Substring() to grab the substrings without the need to reverse each sb instance
This comment has been minimized.
This comment has been minimized.
iSazonov
Feb 27, 2019
Collaborator
Yes, my thoughts was about this and Span<>. If it is too hard for @ece-jacob-scott we can postpone this to follow PR.
This comment has been minimized.
This comment has been minimized.
ece-jacob-scott
Feb 27, 2019
Author
Contributor
No, that makes sense I can work on implementing that in the next couple of days.
This comment has been minimized.
This comment has been minimized.
IISResetMe
Feb 27, 2019
Contributor
Super cool @ece-jacob-scott, feel free to poke me on Twitter (or tag me here) if you need help or have questions about the previous PR for this
This comment has been minimized.
This comment has been minimized.
|
The new behavior was already reviewed and approved by @PowerShell/powershell-committee, so not clear why this was marked for review again. If there is something new to review, please summarize and re-add the label. |
SteveL-MSFT
removed
the
Review - Committee
label
Feb 27, 2019
ece-jacob-scott
requested review from
adityapatwardhan,
anmenaga,
JamesWTruher,
joeyaiello,
PaulHigin and
TravisEz13
as
code owners
Mar 1, 2019
This comment has been minimized.
This comment has been minimized.
|
@TylerLeonhardt @rjmholt Please help @ece-jacob-scott to resolve merge conflicts in right way. |
This comment has been minimized.
This comment has been minimized.
So I was talking to Tyler last night about it and he helped me out however, I'm getting there build errors ever since I merged with the latest master branch. |
This comment has been minimized.
This comment has been minimized.
|
@ece-jacob-scott There is resource errors - you need build with |
ece-jacob-scott
force-pushed the
ece-jacob-scott:split-negative-number
branch
from
627a9d9
to
c96c58e
May 11, 2019
This comment has been minimized.
This comment has been minimized.
|
@ece-jacob-scott just let us know when you're ready for review :) |
This comment has been minimized.
This comment has been minimized.
|
Sorry didn't see this comment. I thought my last solution was a bit verbose so I split it into 2 functions, one for the negative split and one for the regular split. |
| } | ||
| } | ||
|
|
||
| private static object SplitWithPredicate(ExecutionContext context, IScriptExtent errorPosition, IEnumerable<string> content, ScriptBlock predicate, int limit) | ||
| private static object NegativeSplitWithPredicate(ExecutionContext context, IScriptExtent errorPosition, IEnumerable<string> content, ScriptBlock predicate, int limit) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ece-jacob-scott
Jun 1, 2019
Author
Contributor
No, I think that's just how it was when I got the code so I left it. Good call though.
|
LGTM! Thanks so much for your contribution @ece-jacob-scott! |
This comment has been minimized.
This comment has been minimized.
|
Has a doc issue been filed? |
This comment has been minimized.
This comment has been minimized.
Done |
TravisEz13
dismissed
their
stale review
Jun 3, 2019
out of date
TravisEz13
merged commit f222a68
into
PowerShell:master
Jun 3, 2019
6 of 7 checks passed
TravisEz13
added this to the 7.0.0-preview.2 milestone
Jun 3, 2019
TravisEz13
added
the
CL-Engine
label
Jun 3, 2019
This comment has been minimized.
This comment has been minimized.
|
@ece-jacob-scott Thanks for your contribution! |
This comment has been minimized.
This comment has been minimized.
msftbot
bot
commented
Jul 17, 2019
|
Handy links: |



ece-jacob-scott commentedFeb 23, 2019
•
edited by TravisEz13
Fixes #4765
To solve the problem of negative number arguments in the split command I checked that the argument was in fact negative. If the argument is negative then the regex command gets run with the 'RightToLeft' property. This will grab the correct strings from the content and produce the correct output results. Else if the argument number is not negative then the regex is run normally.
I am competing in the HackIllinois event and was mentored by @rjmholt and @TylerLeonhardt
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.[feature]to your commit messages if the change is significant or affects feature tests