Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

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

Closed
wants to merge 12 commits into from
Closed

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

wants to merge 12 commits into from

Conversation

justinvp
Copy link
Contributor

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

@stephentoub stephentoub added area-System.Text.RegularExpressions api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Dec 21, 2014
@akoeplinger akoeplinger mentioned this pull request Dec 23, 2014
Closed
@ellismg
Copy link
Contributor

ellismg commented Jan 6, 2015

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
Copy link
Contributor Author

justinvp commented Jan 6, 2015

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

@justinvp
Copy link
Contributor Author

justinvp commented Jan 6, 2015

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
Copy link
Contributor

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

@terrajobst terrajobst added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jan 14, 2015
@dnfclas
Copy link

dnfclas commented Jan 15, 2015

@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
Copy link
Member

We've reviewed these APIs.

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

@dotnet-bot
Copy link

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
Copy link
Member

@dotnet-bot okay to test

@ellismg
Copy link
Contributor

ellismg commented Jan 15, 2015

@dotnet-bot add to whitelist

weshaggard
weshaggard reviewed Jan 24, 2015
View reviewed changes
src/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexCaptureCollection.cs
void IList<Capture>.Insert(int index, Capture item)
{
throw new NotSupportedException();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

davkean commented Jan 28, 2015

please test this

@justinvp
Implement IList<T>, IReadOnlyList<T>, and IList
d7d1ce6 
Implement IList<T>, IReadOnlyList<T>, and IList on CaptureCollection,
GroupCollection, and MatchCollection.
@justinvp
Add RegexCollectionDebuggerProxy
1f003a4
@justinvp
Add exception message per code review feedback
4104077 
This is the same exception message used by ReadOnlyCollection<T>.
@justinvp justinvp mentioned this pull request Jan 29, 2015
Closed
@nguerrera
Copy link
Contributor

@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
Copy link
Contributor Author

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

@justinvp justinvp closed this Mar 25, 2015
@justinvp justinvp deleted the issue-271 branch October 1, 2015 22:30
@karelz karelz assigned justinvp and unassigned Priya91 Oct 3, 2016
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
Olafski pushed a commit to Olafski/corefx that referenced this pull request Jun 15, 2017
@leecow
Merge pull request dotnet#316 from leecow/master
5f60ab6 
Add link for Windows Server Hosting installer
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@justinvp justinvp

Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews area-System.Text.RegularExpressions
Projects
None yet
Milestone
1.0.0-rtm