GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add char range overload to DotDot operator by IISResetMe · Pull Request #5026 · PowerShell/PowerShell

Conversation

Mathias R. Jessen (IISResetMe) commented Oct 5, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mathias R. Jessen (IISResetMe) requested review from Bruce Payette (BrucePay), Dongbo Wang (daxian-dbw), Jason Shirk (lzybkr) and sergei (vors) as code owners

October 5, 2017 22:39

Microsoft Contribution License Agreements (msftclas) commented Oct 5, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

All CLA requirements met.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Jason Shirk (lzybkr) self-assigned this

Oct 5, 2017

Jason Shirk (lzybkr) suggested changes

Oct 6, 2017

View reviewed changes

Jason Shirk (lzybkr) left a comment

Copy link

Copy Markdown

Contributor

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 code changes look fine - but we need tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Jason Shirk (lzybkr) Oct 6, 2017

Copy link

Copy Markdown

Contributor

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.

What do you think about returning char[] instead?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Oct 25, 2017

Copy link

Copy Markdown

Collaborator

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.

Mathias R. Jessen (@IISResetMe) Could you please address the comment?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mathias R. Jessen (IISResetMe) Oct 25, 2017

Copy link

Copy Markdown

Collaborator Author

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 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Jason Shirk (lzybkr) Oct 6, 2017

Copy link

Copy Markdown

Contributor

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 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.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mathias R. Jessen (IISResetMe) Oct 6, 2017

Copy link

Copy Markdown

Collaborator Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Oct 24, 2017

Copy link

Copy Markdown

Collaborator

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.

Mathias R. Jessen (@IISResetMe) Is this addresed?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mathias R. Jessen (IISResetMe) Oct 24, 2017

Copy link

Copy Markdown

Collaborator Author

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've removed the redundant branch in both overloads now. 0406cb3

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) reviewed

Oct 10, 2017

View reviewed changes

Comment thread

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

Ilya (iSazonov) Oct 10, 2017

Copy link

Copy Markdown

Collaborator

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.

Will this work:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mathias R. Jessen (IISResetMe) Oct 10, 2017

Copy link

Copy Markdown

Collaborator Author

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, this will produce the sequence A (0x41) through Z (0x5A)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iSazonov reacted with thumbs up emoji

All reactions

👍 1 reaction

Kris Borowinski (kborowinski) Jan 21, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mark Kraus (markekraus) Jan 21, 2018

Copy link

Copy Markdown

Contributor

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.

Kris Borowinski (@kborowinski) there is an open issue for that #5519

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Kris Borowinski (kborowinski) Jan 21, 2018

Copy link

Copy Markdown

Contributor

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.

Mark Kraus (@markekraus): sorry, I've missed that issue

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Ilya (iSazonov) Oct 10, 2017

Copy link

Copy Markdown

Collaborator

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 wonder - is this ASCII Arithmetic work correctly for unicode?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) Oct 10, 2017

Copy link

Copy Markdown

Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Oct 10, 2017

Copy link

Copy Markdown

Collaborator

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 we added `u{} should we detect surrogate pairs and write an error?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) Oct 10, 2017

Copy link

Copy Markdown

Contributor

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 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Oct 11, 2017

Copy link

Copy Markdown

Collaborator

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 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Oct 24, 2017

Copy link

Copy Markdown

Collaborator

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.

Mathias R. Jessen (@IISResetMe) Could you please address the comment in code or add relevant comments in the PR description for future documentation?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mathias R. Jessen (IISResetMe) Oct 24, 2017

Copy link

Copy Markdown

Collaborator Author

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.

Ilya (@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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Oct 25, 2017

Copy link

Copy Markdown

Collaborator

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.

Thanks.

Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mathias R. Jessen (IISResetMe) requested review from James Truher (JamesWTruher), Paul Higinbotham (PaulHigin), Travis Plunk (TravisEz13), Aditya Patwardhan (adityapatwardhan), Andrew (anmenaga) and Dan Travison (dantraMSFT) as code owners

October 15, 2017 22:18

Mathias R. Jessen (IISResetMe) commented Oct 15, 2017

Copy link

Copy Markdown

Collaborator Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mark Kraus (markekraus) commented Oct 15, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Mathias R. Jessen (@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.

IISResetMe and TravisEz13 reacted with hooray emoji

All reactions

🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mathias R. Jessen (IISResetMe) added 2 commits

October 16, 2017 01:43


          Add char range overload to DotDot operator

339e6fd

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'


          Add RangeOperator.Tests.ps1

d54f646

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

Mathias R. Jessen (IISResetMe) commented

Oct 15, 2017

View reviewed changes

Mathias R. Jessen (IISResetMe) left a comment

Copy link

Copy Markdown

Collaborator Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mathias R. Jessen (IISResetMe) commented Oct 15, 2017

Copy link

Copy Markdown

Collaborator Author

Thanks Mark Kraus (@markekraus), that seems to have done the trick.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          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

Travis Plunk (TravisEz13) commented Oct 16, 2017

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          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

Mathias R. Jessen (IISResetMe) commented Oct 21, 2017

Copy link

Copy Markdown

Collaborator Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) reviewed

Oct 21, 2017

View reviewed changes

Comment thread

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]

Ilya (iSazonov) Oct 21, 2017

Copy link

Copy Markdown

Collaborator

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Oct 24, 2017

Copy link

Copy Markdown

Collaborator

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.

Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

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

Ilya (iSazonov) Oct 21, 2017

Copy link

Copy Markdown

Collaborator

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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


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

Ilya (iSazonov) Oct 21, 2017

Copy link

Copy Markdown

Collaborator

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.

Formatting - please add space after |

Please add type check as in previous test.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

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

Ilya (iSazonov) Oct 21, 2017

Copy link

Copy Markdown

Collaborator

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.

Please remove.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

		$Range[0] \| Should Be 0
		}

		It "Range operator works in ascending and descending order" {

Ilya (iSazonov) Oct 21, 2017

Copy link

Copy Markdown

Collaborator

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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


		Context "Range operator operand types" {

		It "Range operator works on [int]" {

Ilya (iSazonov) Oct 21, 2017

Copy link

Copy Markdown

Collaborator

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

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

Ilya (iSazonov) Oct 21, 2017

Copy link

Copy Markdown

Collaborator

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mathias R. Jessen (IISResetMe) Oct 21, 2017

Copy link

Copy Markdown

Collaborator Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Oct 21, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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.

Thanks for clarify. Please remove the confuse test.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Oct 24, 2017

Copy link

Copy Markdown

Collaborator

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mathias R. Jessen (IISResetMe) Oct 24, 2017

Copy link

Copy Markdown

Collaborator Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

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

Ilya (iSazonov) Oct 21, 2017

Copy link

Copy Markdown

Collaborator

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mathias R. Jessen (IISResetMe) Oct 21, 2017

Copy link

Copy Markdown

Collaborator Author

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 existing numeric range operator only supports int regardless of input type

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

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

Ilya (iSazonov) Oct 21, 2017

Copy link

Copy Markdown

Collaborator

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

		$Range[9] \| Should Be 10
		}
		}
		} No newline at end of file

Ilya (iSazonov) Oct 21, 2017

Copy link

Copy Markdown

Collaborator

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.

Please add new line at EOF.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Update RangeOperator.Tests.ps1

83429ef

Fixed formatting, removed unnecessary checks

Travis Plunk (TravisEz13) commented Oct 23, 2017

Copy link

Copy Markdown

Member

Jason Shirk (@lzybkr) Can you update your review?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Travis Plunk (TravisEz13) commented Oct 23, 2017

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) commented Oct 24, 2017

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          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

Mathias R. Jessen (IISResetMe) commented Oct 24, 2017

Copy link

Copy Markdown

Collaborator Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) commented Oct 25, 2017

Copy link

Copy Markdown

Collaborator

Mathias R. Jessen (@IISResetMe) Many thanks! We are near to merge. Please address Jason Shirk (@lzybkr) comment above.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sergei (vors) removed their request for review

October 25, 2017 06:02

Mathias R. Jessen (IISResetMe) commented Oct 25, 2017

Copy link

Copy Markdown

Collaborator Author

Ilya (@iSazonov) Done

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) commented Oct 25, 2017

Copy link

Copy Markdown

Collaborator

Jason Shirk (@lzybkr) Could you please take another look if you have a bit free time?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mathias R. Jessen (IISResetMe) mentioned this pull request

Oct 25, 2017

Enhance -split operator with RightToLeft splitting #5125

Closed

Jason Shirk (lzybkr) approved these changes

Oct 26, 2017

View reviewed changes

Ilya (iSazonov) approved these changes

Oct 27, 2017

View reviewed changes

Travis Plunk (TravisEz13) merged commit 9b32c1d into PowerShell:master

Oct 28, 2017

Ilya (iSazonov) added the Documentation Needed in this repo Documentation is needed in this repo label

Oct 29, 2017

Mathias R. Jessen (IISResetMe) deleted the overload-DotDot-CharRange-1 branch

November 17, 2017 19:29

Tim Curwick (TimCurwick) mentioned this pull request

Nov 21, 2017

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

Closed

Joey Aiello (joeyaiello) mentioned this pull request

Oct 15, 2018

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

Closed

Joey Aiello (joeyaiello) removed Documentation Needed in this repo Documentation is needed in this repo labels

Oct 15, 2018

Ilya (iSazonov) mentioned this pull request

Jan 6, 2020

Range Operator work with date and ip #11489

Closed

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Add char range overload to DotDot operator (PowerShell#5026)

b481351

Reviewers

Ilya (iSazonov)

iSazonov approved these changes

Bruce Payette (BrucePay)

Awaiting requested review from BrucePay

Dongbo Wang (daxian-dbw)

Awaiting requested review from daxian-dbw

daxian-dbw is a code owner

Aditya Patwardhan (adityapatwardhan)

Awaiting requested review from adityapatwardhan

Andrew (anmenaga)

Awaiting requested review from anmenaga

Dan Travison (dantraMSFT)

Awaiting requested review from dantraMSFT

James Truher (JamesWTruher)

Awaiting requested review from JamesWTruher

Paul Higinbotham (PaulHigin)

Awaiting requested review from PaulHigin

Travis Plunk (TravisEz13)

Awaiting requested review from TravisEz13

+3 more reviewers

Mark Kraus (markekraus)

markekraus left review comments

Kris Borowinski (kborowinski)

kborowinski left review comments

Jason Shirk (lzybkr)

lzybkr approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Jason Shirk (lzybkr)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

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

Uh oh!

Uh oh!

Microsoft Contribution License Agreements (msftclas) commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jason Shirk (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!

Kris Borowinski (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!

Mathias R. Jessen (IISResetMe) commented Oct 15, 2017

Uh oh!

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

Uh oh!

Uh oh!

Mathias R. Jessen (IISResetMe) left a comment

Choose a reason for hiding this comment

Uh oh!

Mathias R. Jessen (IISResetMe) commented Oct 15, 2017

Uh oh!

Travis Plunk (TravisEz13) commented Oct 16, 2017

Uh oh!

Mathias R. Jessen (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!

Mathias R. Jessen (IISResetMe) commented Oct 5, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Microsoft Contribution License Agreements (msftclas) commented Oct 5, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Kris Borowinski (kborowinski) Jan 21, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Mark Kraus (markekraus) commented Oct 15, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) Oct 21, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.