Implement IList<T>, IReadOnlyList<T>, and IList on Regex Collections

[justinvp]

Fixes #271

This is still a work-in-progress.

TODO

• Implement IList<T>, IReadOnlyList<T>, and IList
• Add RegexCollectionDebuggerProxy
• API review
• Changes based on API review and initial code review feedback
• Cleanup
• Add tests
• Final code review

Questions/Notes

• As @sharwell noted in Update regular expressions collections to implement IReadOnlyList<T> #277: GroupEnumerator is internal, so it might be possible to change the type of GroupEnumerator.Capture from Capture to Group. This would make the explicit cast in the generic Current property unnecessary.

[ellismg]

Hi @justinvp. I'm going to use this PR and corresponding issue as a way to test out the process outlined in #294. Do you mind ensuring the PR and issue are up to date with your current thinking on the proposal?

I've preemptively requested an API review slot for next Wednesday (1/14) where we can discuss this if it's ready to go.

If you need any help or have any questions about the process please do let me know. Thanks!

[justinvp]

@ellismg Thanks. The issue and this PR are good to go for the API review.

[justinvp]

By the way, the AppVeyor build failed after the last commit due to an unrelated issue:

The process cannot access the file 'C:\projects\corefx\src\packages\xunit.core.2.0.0-beta5-build2785\xunit.core.2.0.0-beta5-build2785.nupkg' because it is being used by another process.

[FiveTimesTheFun]

Hi @justinvp , thanks for the PR, I've restarted AppVeyor for you.

[dnfclas]

@justinvp, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[terrajobst]

We've reviewed these APIs.

Overall, it looks good but there is some feedback that needs to be addressed.

[dotnet-bot]

Can one of the admins approve this PR test or whitelist this user? ('@dotnet-bot add to whitelist' to whitelist, '@dotnet-bot okay to test' to accept for PR, '@dotnet-bot test this please' to retest. Case sensitive).

[terrajobst]

@dotnet-bot okay to test

[ellismg]

@dotnet-bot add to whitelist

[weshaggard]

We should probably add some exception text to these NotSupportedExceptions. Follow the pattern of what something like ReadOnlyCollection throws.

[justinvp]

The reason I didn't include exception text was because I was just following the immutable collections, which don't include any exception text when throwing NotSupportedException. If we want the exception text, I can add it. And if so, we should probably open a bug to do the same for the immutable collections.

See

corefx/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList`1.cs

Line 1151 in dd703af

throw new NotSupportedException();

[KrzysztofCwalina]

I think this component should follow the conventions used by the larger framework, i.e. include exception message. Both for consistency, and so we can explain in the message how to test for whether these operations are supported or not (point people to the IsReadOnly property).

[davkean]

please test this

[nguerrera]

@justinvp. We've changed our branching strategy in a way that impacts this PR.

When you're ready to continue work on this, you'll need to either recreate the PR against the 'future' branch or wait until after we have snapped the first release branch before we can merge it in to master.

See https://github.com/dotnet/corefx/wiki/branching-guide

[justinvp]

Closing this. I'll open a new PR against the 'future' branch.