Skip to content

Conversation

@sharwell
Copy link
Contributor

Review commit-by-commit encouraged 😄

@sharwell sharwell requested a review from a team as a code owner July 16, 2020 02:05
Copy link
Contributor

@jmarolf jmarolf left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this! Question is not blocking, since cleaning this up first is clearly necessary!

public static TInterfaceType GetService<TInterfaceType, TServiceType>(this IServiceProvider sp)
where TInterfaceType : class
where TServiceType : class
/// <inheritdoc cref="Shell.ServiceExtensions.GetService{TService, TInterface}(IServiceProvider, bool)"/>
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we're not just using the one that comes from the Shell binaries in the first place? Will this also inherit a documentation that won't apply for whatever that bool parameter is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any reason we're not just using the one that comes from the Shell binaries in the first place?

It uses ThreadHelper.JoinableTaskFactory. I'm not sure we initialize that property in test scenarios.

Will this also inherit a documentation that won't apply for whatever that bool parameter is?

Not really, it just ignores it.

@sharwell sharwell merged commit 4449555 into dotnet:master Jul 16, 2020
@ghost ghost added this to the Next milestone Jul 16, 2020
@sharwell sharwell deleted the get-service branch July 16, 2020 20:50
@JoeRobich JoeRobich modified the milestones: Next, 16.8.P1 Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants