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

Simplified Position API #1990

Merged
merged 1 commit into from
Dec 20, 2017
Merged

Conversation

KrzysztofCwalina
Copy link
Member

@@ -16,20 +16,19 @@ namespace System.Collections.Sequences

public static implicit operator Position(int index) => new Position(index, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need all these operators, Get<T>, and static Create method?
Can we just expose item and index and add a single public constructor that takes them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also in all of the usages of Get<T> you never check that item is not null. Do we need to add some kind of validation that position was of an expected type?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's how it was originally, but you guys wanted to make it harder for people to get access to the object. Get/Create allow you to store an internal type in object to make it impossible to retrieve by third party code.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can always do Get<object> and get around it. And ReadOnlyBuffer stores publicly available types there. I don't think we can protect it from retrieving the item.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I wanted to change the check to only return the object if typeof(T) == _item.GetType().
I was planing to do it in a subsequent PR.

@KrzysztofCwalina
Copy link
Member Author

Let me merge this one. I will play with the typeof(T) == _item.GetType() check, and if it does not work, we can go back to Item and Index properties.

@KrzysztofCwalina KrzysztofCwalina merged commit 0686756 into dotnet:master Dec 20, 2017
@KrzysztofCwalina KrzysztofCwalina deleted the Position branch January 3, 2018 00:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants