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

Refactoring ServiceCollection #123

Closed
wants to merge 1 commit into from
Closed

Refactoring ServiceCollection #123

wants to merge 1 commit into from

Conversation

pranavkm
Copy link
Contributor

  • Remove use of IConfiguration from ServiceCollection
  • Removing the Add* methods from IServiceCollection that don't accept an
    IServiceDescriptor

Fixes #116 #117

@pranavkm
Copy link
Contributor Author

cc @halter73

rynowak
rynowak reviewed Oct 17, 2014
View reviewed changes
src/Microsoft.Framework.DependencyInjection/ServiceCollection.cs
{
}

public ServiceCollection(IConfiguration configuration)
Copy link
Member

Choose a reason for hiding this comment

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

what's the replacement for this? We're using it in our functional tests to do dirty, filthy, despicable things.

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 only use case for passing IConfiguration into ServiceCollection is to be able to replace registered services with types specified from config - https://github.com/aspnet/DependencyInjection/blob/dev/src/Microsoft.Framework.DependencyInjection/ServiceDescriber.cs#L125-L133. We don't use this feature in Mvc's functional tests.

Copy link
Member

Choose a reason for hiding this comment

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

we absolutely do https://github.com/aspnet/Mvc/blob/dev/test/Microsoft.AspNet.Mvc.FunctionalTests/TestHelper.cs#L51

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rynowak We don't depend on the ServiceCollection taking a IConfiguration in Mvc. The behavior should remain unchanged once we make this change.

@pranavkm
Copy link
Contributor Author

@halter73 updated with NotNull attributes like we discussed

halter73
halter73 reviewed Oct 21, 2014
View reviewed changes
src/Microsoft.Framework.DependencyInjection/IServiceCollection.cs
/// <summary>
/// Adds a sequence of <see cref="IServiceDescriptor"/> to this instance.
/// </summary>
/// <param name="descriptor">The <see cref="IEnumerable{T}"/> or <see cref="IServiceDescriptor"/>s to add.</param>
Copy link
Member

Choose a reason for hiding this comment

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

Is "or" supposed to be "of"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@halter73
Copy link
Member

Once my comments are addressed, :shipit:.

@pranavkm
Refactoring ServiceCollection
cfa2cc6 
* Remove use of IConfiguration from ServiceCollection
* Removing the Add* methods from IServiceCollection that don't accept an
  IServiceDescriptor

Fixes #116 #117
@pranavkm pranavkm closed this Oct 27, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pranavkm @halter73 @rynowak