Skip to content

Conversation

@rayao
Copy link
Contributor

@rayao rayao commented Jan 31, 2016

Use Micorsoft DI framework to replace current Dictionary<,> based service registration.
This is a relatively aggressive change, compared to my previous PR. There's an option 2 #286 pull request which is basically identical to the previous PR #264. It's up to you to make a choice.
In this change the main difference is that class ApiBuilder is added for API service registration, and later build an ApiConfiguration.
Pros:

  • No Committed state for ApiConfiguration, it's committed once created. For now those related methods are not removed yet but they do nothing.
  • ApiConfiguration itself is registered as a service and participate in DI. That means a descendant Api class could register some service which gets an ApiConfiguration injected on construction.

But of course, at the same time this change inevitably has broader impact to existing code.

  • ApiConfiguration public interface changes, This breaks a lot of things, which of course are fixed with this change. But, any consumer out there may have a bigger chance to break, compared to option 2.

@lewischeng-ms
Copy link
Contributor

@rayao Thanks so much for the PR. Personally I also prefer your option 1: it makes much sense to treat ApiConfiguration as a "service" and manage all services consistently in DI. Since now ApiConfiguration itself participates in DI, ApiBuilder plays an important role on decoupling ApiConfiguration from other services (hook handlers). Furthermore I would suggest we do this more aggressively and completely to make the API cleaner. I will comment more on some specific lines.

@rayao
Copy link
Contributor Author

rayao commented Feb 4, 2016

@lewischeng-ms Thank you Lewis, really happy that we have common thoughts. Since Spring Festival is approaching, if you're going home before the official holiday, don't urge yourself to finish this PR before you leaving, just go at your own pace, it's OK for me to wait. I just want to be helpful and really hate to become a burden for you and your team. :)
And I'll work till the official holiday.

/// The <see cref="ApiBuilder"/> with which to create an <see cref="ApiConfiguration"/>.
/// </param>
/// <returns>The <see cref="ApiBuilder"/>.</returns>
[CLSCompliant(false)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest a more concise name ConfigureApi because we are actually configuring and finally building an Api here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks. also If any other naming is inappropriate please let me know.

@lewischeng-ms
Copy link
Contributor

@rayao Apart from issue #289, #290, #291 and the design proposal about the inner handler, would you please change your PR accordingly and merge first? For the issues mentioned and subsequent improvement, we can do this gradually by sending new PRs and reviewing together. This BIG PR really makes me feel hard to review and merge other fixes... Thanks:-)

@rayao
Copy link
Contributor Author

rayao commented Feb 17, 2016

Ok, thanks!

Lei Yao added 2 commits February 17, 2016 22:07
@rayao
Copy link
Contributor Author

rayao commented Feb 17, 2016

Pushed with c7338fe. Rolling build passed. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants