-
Notifications
You must be signed in to change notification settings - Fork 137
Microsoft.FX.DI integration [Option 1] #285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…vice registration.
|
@rayao Thanks so much for the PR. Personally I also prefer your option 1: it makes much sense to treat |
|
@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. :) |
| /// The <see cref="ApiBuilder"/> with which to create an <see cref="ApiConfiguration"/>. | ||
| /// </param> | ||
| /// <returns>The <see cref="ApiBuilder"/>.</returns> | ||
| [CLSCompliant(false)] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@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:-) |
|
Ok, thanks! |
Minimize usage of CLSCompliant; Concise naming;
|
Pushed with c7338fe. Rolling build passed. :) |
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:
But of course, at the same time this change inevitably has broader impact to existing code.