Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

Reduce IFeatureCollection surface area. #356

Merged
merged 1 commit into from
Aug 3, 2015
Merged

Reduce IFeatureCollection surface area. #356

merged 1 commit into from
Aug 3, 2015

Conversation

Tratcher
Copy link
Member

@muratg
Copy link

muratg commented Jul 28, 2015

Does this break other repos?

@Tratcher
Copy link
Member Author

Just the servers, and those should be easy to fix.

}
public bool Equals(KeyValuePair<Type, object> x, KeyValuePair<Type, object> y)
{
return x.Key.Equals(y.Key);
Copy link

Choose a reason for hiding this comment

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

Just comparing keys is enough to call these "Equals"? It feels a little weird, i.e. {"x":1} and {"x":2} would be equal per this.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only used on line 77 to filter out features that exist in both collections, so we only want to match by key.

Copy link

Choose a reason for hiding this comment

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

It's public. It may only be used by this class for that purpose, but what if others choose to use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the KeyComparer class is private.

Copy link

Choose a reason for hiding this comment

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

So it is. Never mind. :)

@Tratcher
Copy link
Member Author

Updated to lazy initialize the feature storage. In the average case we'll only be reading from the default/fallback, not making changes.

@Tratcher
Copy link
Member Author

@Tratcher
Copy link
Member Author

Re: aspnet/HttpSysServer@161fdbb

@Tratcher
Copy link
Member Author

@Eilon to review, but we don't want to check in the break until we get Helios building again.

@Eilon
Copy link
Contributor

Eilon commented Jul 31, 2015

:shipit: from me. If you wanted to avoid breaking changes you could probably keep the .Add method at first, check this change in, then update Kestrel and WebListener to use the indexer, and finally remove the .Add method. But I don't care much either way.

@Tratcher
Copy link
Member Author

Tratcher commented Aug 3, 2015

@Eilon it doesn't matter if I break Helios now, Andrew checked in a breaking change to ILibraryManager on Friday so the old Helios assemblies won't run anymore.

@Eilon
Copy link
Contributor

Eilon commented Aug 3, 2015

@Tratcher thanks, I'm fine either way.

@Tratcher
Copy link
Member Author

Tratcher commented Aug 3, 2015

@Tratcher
Copy link
Member Author

Tratcher commented Aug 3, 2015

FYI: Updated WebListener to use the same style static feature collection as Helios: aspnet/HttpSysServer@e69cb5c

@Tratcher Tratcher merged commit ac945a0 into dev Aug 3, 2015
@Tratcher Tratcher deleted the Features branch August 3, 2015 22:00
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.

5 participants