Add char range overload to DotDot operator by IISResetMe · Pull Request #5026 · PowerShell/PowerShell

IISResetMe · 2017-10-05T22:39:04Z

This PR attempts to allow the DotDot operator (..) to generate
character ranges, in turn allowing shorthand syntax like:

$AtoZ = -join('A'..'Z')

Since PowerShell has no shorthand syntax for char literals, we
test whether the left-hand-side is of type String, and subsequently
attempts to convert both operands to a char (in accordance with
PowerShell operator overload behavior in general).

NOTE:

This breaks a number of currently supported range operations, such
as implicit numerical conversion of strings:
'1'..'100'
This implementation is fairly naive, making no attempts to assert whether the input characters (or any codepoints in between) fall outside UCS-2 (ie. surrogates). Attempts to expand ranges bound by surrogate pairs are expected to fail and not covered by this implementation (I will leave this an exercise for someone more experienced with unicode hackery than myself)

I was unable to locate existing tests for the integer range function, please advise on where to add those.

msftclas · 2017-10-05T22:39:12Z

All CLA requirements met.

lzybkr

The code changes look fine - but we need tests.

lzybkr · 2017-10-06T00:02:12Z

src/System.Management.Automation/engine/runtime/Operations/NumericOps.cs

What do you think about returning char[] instead?

@IISResetMe Could you please address the comment?

I like the idea, but for now I think we should keep operator output consistent (ie. return object[] with individual items of the expected type).

There are are number of builtin operators that return object[] in this fashion (@(), -split, any comparison operator in array/filter mode etc.), changing the behavior should be a concerted effort across all of them IMO

lzybkr · 2017-10-06T00:03:17Z

src/System.Management.Automation/engine/runtime/Operations/NumericOps.cs

I think this special case is correctly handled by the code below and it happens rarely, so maybe we should remove it. (And I see you copied the code from the int version, so you could remove it there too.)

Mainly did it this way to keep it similar to the existing code in Compiler.cs. I'm inclined to make a single Range method that takes the operands as Expression's, de-duplicate the loop that generates the actual sequence and then cast the entire thing to char[], int[] or long[] based on the lhs operand before returning. Will update the code later today

@IISResetMe Is this addresed?

I've removed the redundant branch in both overloads now. 0406cb3

iSazonov · 2017-10-10T07:56:02Z

src/System.Management.Automation/engine/parser/Compiler.cs

Will this work:

'A'..'Z' | % { ... }

Yes, this will produce the sequence A (0x41) through Z (0x5A)

Apparently it does not work on the Powershell Core 6.0.0 GA

> 'A'..'Z' | % {$_} Cannot convert value "A" to type "System.Int32". Error: "Input string was not in a correct format." At line:1 char:1 + 'A'..'Z' | % {$_} + ~~~~~~~~~~~~~~~~~~ + CategoryInfo : InvalidArgument: (:) [], RuntimeException + FullyQualifiedErrorId : InvalidCastFromStringToInteger

@kborowinski there is an open issue for that #5519

@markekraus: sorry, I've missed that issue

iSazonov · 2017-10-10T07:57:49Z

src/System.Management.Automation/engine/runtime/Operations/NumericOps.cs

I wonder - is this ASCII Arithmetic work correctly for unicode?

It won't work with surrogate pairs because those can't be represented in a char, but I don't think that is an interesting scenario.

Since we added `u{} should we detect surrogate pairs and write an error?

I guess we should error on invalid characters (half of the pair). We could also support surrogate pair ranges if we didn't convert the string to char immediately, instead analyzing the string more thoroughly. But I'm not sure I'd block this PR on either of those.

I guess it will work with traditional languages in common scenarios but before I saw issues from China and Japan mans - if the new operator does not matter to them I agree with you.

@IISResetMe Could you please address the comment in code or add relevant comments in the PR description for future documentation?

@iSazonov I don't feel confident enough in my unicode-fu to attempt to implement the appropriate handling in code for this at the moment, I've updated the PR description to reflect this deficiency instead.

Thanks.

Closed.

IISResetMe · 2017-10-15T22:23:35Z

Oops, rebased powershell:master into the branch, looks like all parent commits are in the PR now, how do I clean it up? 😞

markekraus · 2017-10-15T23:16:19Z

@IISResetMe did you only have the 2 commits? if so you could do this (assuming PowerShell is the name of your upstream remote for the actual PowerShell repo). Make sure you stash or commit any workspace changes before doing this as they will be lost. I'm not a git expert, and maybe there is a better way. So maybe wait until someone more seasoned either "thumbs up" this or tells you just how wrong I am :)

git fetch
git checkout -b 20171015fix PowerShell/master
git cherry-pick 6e6dfffc2d0150b504585597ad4df997a174867a
git cherry-pick 6d6f20d28157ee49071f018051ac7b35be57bbb0
git checkout -b 20171015backup overload-DotDot-CharRange-1
git checkout overload-DotDot-CharRange-1
git reset --hard 20171015fix
git push --force

This will create a new 20171015fix branch at the current master on this repo, then cherry-picks your 2 commits, then backs up your existing branch to 20171015backup, and then hard reset your branch to the 20171015fix branch. This will both "rebase" your branch on to master and add your commits with out the merge mess. This will likely mess up any comment history here, but I guess it's already some what messed up.

This commit attempts to allow the DotDot operator (..) to generate character ranges. Since PowerShell has no shorthand syntax for char literals, this change tests whether the left-hand-side is of type String, and subsequently attempts to convert both operands to a char. NOTE: This breaks a number of currently supported range operations, such as implicit numerical conversion of strings: '1'..'100'

This commit adds basic tests for the current integer range (..) operator

IISResetMe

These were the most basic tests I could think of for the existing Range operator. All examples I could find in the about_* help files pass these.

IISResetMe · 2017-10-15T23:51:22Z

Thanks @markekraus, that seems to have done the trick.

Add tests for the [char] overload feature. RangeOperator.Tests.ps1 encoding changed to UTF16LE (Windows "Unicode") to allows inline unicode chars in test case

TravisEz13 · 2017-10-16T22:27:49Z

Can you make sure RangeOperator.Tests.ps1 is UTF8 without BOM? It shows up as binary.

Changed RangeOperator.Tests.ps1 encoding to UTF-8 (no BOM), substituted UTF-16 char literals with [char] casts

IISResetMe · 2017-10-21T17:08:08Z

@TravisEz13 Done, https://github.com/IISResetMe/PowerShell/blob/34bc59046698d0de41bb5403a6105c7070b38a46/test/powershell/Language/Operators/RangeOperator.Tests.ps1 is UTF-8 (with updated tests) now

iSazonov · 2017-10-21T20:00:15Z

test/powershell/Language/Operators/RangeOperator.Tests.ps1

+        It "Range operator generates arrays of integers" {
+            $Range = 5..8
+            $Range.count | Should Be 4
+            $Range[0] | Should BeOfType [int]


Please add tests for values - $Range[0] | Should Be 5

iSazonov · 2017-10-21T20:02:22Z

test/powershell/Language/Operators/RangeOperator.Tests.ps1

+        It "Range operator accepts negative integer values" {
+            {
+                -8..-5
+            } |Should Not Throw


Please remove the should - next line do the same and Pester can catch a throw there correctly.

iSazonov · 2017-10-21T20:03:48Z

test/powershell/Language/Operators/RangeOperator.Tests.ps1

+
+            $Range = -8..-5            
+            $Range.count | Should Be 4
+            $Range[0] |Should Be -8


Formatting - please add space after |

Please add type check as in previous test.

iSazonov · 2017-10-21T20:04:06Z

test/powershell/Language/Operators/RangeOperator.Tests.ps1

+        It "Range operator support single-item sequences" {
+            {
+                0..0
+            } |Should Not Throw


Please remove.

iSazonov · 2017-10-21T20:09:01Z

test/powershell/Language/Operators/RangeOperator.Tests.ps1

+            $Range[0] | Should Be 0
+        }
+
+        It "Range operator works in ascending and descending order" {


Previous tests already test "ascending". I think we could test here only descending order for positive and negative numbers. But I believe we should replace some tests with one - "-2 .. 2" and "2 .. -2" - so we check transition through zero too.

iSazonov · 2017-10-21T20:17:21Z

test/powershell/Language/Operators/RangeOperator.Tests.ps1

+
+    Context "Range operator operand types" {
+
+        It "Range operator works on [int]" {


Please remove the test - we do the checks above already.

iSazonov · 2017-10-21T20:19:56Z

test/powershell/Language/Operators/RangeOperator.Tests.ps1

+            } | Should Not Throw
+            $Range = ([long]1)..([long]10)
+            $Range.count | Should Be 10
+            $Range.Where({$_ -is [int]}).count | Should Be 10


Is it really [int]? I think we should use values > int.MaxValue.
And please check values too.

Current implementation doesn't support values > int.MaxValue, intended to add this in a separate branch/PR.

Thanks for clarify. Please remove the confuse test.

@IISResetMe Please address the comment about the tests for [long] and [bigint].
We should remove its - they don't make sense.

You're right, they don't make too much sense at the moment. I've removed them now

iSazonov · 2017-10-21T20:20:56Z

test/powershell/Language/Operators/RangeOperator.Tests.ps1

+            } | Should Not Throw
+            $Range = ([bigint]1)..([bigint]10)
+            $Range.count | Should Be 10
+            $Range.Where({$_ -is [int]}).count | Should Be 10


Is it really [int]? I think we should use values > long.MaxValue.
And please check values too.

The existing numeric range operator only supports int regardless of input type

iSazonov · 2017-10-21T20:22:12Z

test/powershell/Language/Operators/RangeOperator.Tests.ps1

+            {
+                1.2d..2.9d
+            } | Should Not Throw
+            $Range = 1.1d..9.9d


Why so much? I think enough $Range = 1.1d..3.9d

iSazonov · 2017-10-21T20:23:58Z

test/powershell/Language/Operators/RangeOperator.Tests.ps1

+            $Range[9] | Should Be 10
+        }
+    }
+}


Please add new line at EOF.

Fixed formatting, removed unnecessary checks

TravisEz13 · 2017-10-23T17:48:33Z

@lzybkr Can you update your review?

TravisEz13 · 2017-10-23T17:50:04Z

@iSazonov Can you indicate if you think this is ready or not (feel free to say if someone else signs off)?

iSazonov · 2017-10-24T06:46:52Z

@TravisEz13 I updated three comment above - after they have been resolved we'll ready to merge.

Both Range() overloads have unnecessary branches for the special case of lhs == rhs, removed for simplicity.

Removed tests for .. using [long] and [bigint] input operands. I believe these will make more sense once we add support for ranges with values outside Int32, but for now might be confusing

IISResetMe · 2017-10-24T21:03:45Z

@iSazonov I believe I've addressed all outstanding comments :-)

iSazonov · 2017-10-25T05:29:24Z

@IISResetMe Many thanks! We are near to merge. Please address @lzybkr comment above.

IISResetMe · 2017-10-25T07:39:02Z

@iSazonov Done

iSazonov · 2017-10-25T07:51:22Z

@lzybkr Could you please take another look if you have a bit free time?

IISResetMe requested review from BrucePay, daxian-dbw, lzybkr and vors as code owners October 5, 2017 22:39

lzybkr self-assigned this Oct 5, 2017

lzybkr suggested changes Oct 6, 2017

View reviewed changes

iSazonov reviewed Oct 10, 2017

View reviewed changes

IISResetMe requested review from JamesWTruher, PaulHigin, TravisEz13, adityapatwardhan, anmenaga and dantraMSFT as code owners October 15, 2017 22:18

IISResetMe added 2 commits October 16, 2017 01:43

Add RangeOperator.Tests.ps1

d54f646

This commit adds basic tests for the current integer range (..) operator

IISResetMe commented Oct 15, 2017

View reviewed changes

Change RangeOperator.Tests.ps1 encoding, add tests

3f82190

Add tests for the [char] overload feature. RangeOperator.Tests.ps1 encoding changed to UTF16LE (Windows "Unicode") to allows inline unicode chars in test case

Change RangeOperator.Tests.ps1 encoding to UTF-8

34bc590

Changed RangeOperator.Tests.ps1 encoding to UTF-8 (no BOM), substituted UTF-16 char literals with [char] casts

iSazonov reviewed Oct 21, 2017

View reviewed changes

Update RangeOperator.Tests.ps1

83429ef

Fixed formatting, removed unnecessary checks

Remove redundant code from Range()

0406cb3

Both Range() overloads have unnecessary branches for the special case of lhs == rhs, removed for simplicity.

Remove confusing/unnecessary RangeOp tests

5afbd6e

Removed tests for .. using [long] and [bigint] input operands. I believe these will make more sense once we add support for ranges with values outside Int32, but for now might be confusing

vors removed their request for review October 25, 2017 06:02

IISResetMe mentioned this pull request Oct 25, 2017

Enhance -split operator with RightToLeft splitting #5125

Closed

lzybkr approved these changes Oct 26, 2017

View reviewed changes

iSazonov approved these changes Oct 27, 2017

View reviewed changes

TravisEz13 merged commit 9b32c1d into PowerShell:master Oct 28, 2017

iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Oct 29, 2017

IISResetMe deleted the overload-DotDot-CharRange-1 branch November 17, 2017 19:29

TimCurwick mentioned this pull request Nov 21, 2017

Range operator (aka DotDot operator) with char operands in pipeline throws error #5519

Closed

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Add char range overload to DotDot operator' MicrosoftDocs/PowerShell-Docs#3080

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

iSazonov mentioned this pull request Jan 6, 2020

Range Operator work with date and ip #11489

Closed


		Context "Range operator operand types" {

		It "Range operator works on [int]" {

Conversation

IISResetMe commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lzybkr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kborowinski Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IISResetMe commented Oct 15, 2017

Uh oh!

markekraus commented Oct 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IISResetMe left a comment

Choose a reason for hiding this comment

Uh oh!

IISResetMe commented Oct 15, 2017

Uh oh!

TravisEz13 commented Oct 16, 2017

Uh oh!

IISResetMe commented Oct 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IISResetMe commented Oct 5, 2017 •

edited

Loading

msftclas commented Oct 5, 2017 •

edited

Loading

kborowinski Jan 21, 2018 •

edited

Loading

markekraus commented Oct 15, 2017 •

edited

Loading

iSazonov Oct 21, 2017 •

edited

Loading