Skip to content
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

Keyed Dictionary Support #15100

Closed
wants to merge 4 commits into from
Closed

Keyed Dictionary Support #15100

wants to merge 4 commits into from

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Jan 15, 2024

This replaced PR #15100, it's a much simpler and more efficient way to support a keyed dictionary. Tested through unit tests as well as running OC web application everything is working as expected

@MikeAlhayek could you please try it on the SMS providers, and let me know if there're a missing corner cases

/cc @sebastienros

@hishamco hishamco requested a review from jtkech as a code owner January 15, 2024 21:09
@hishamco hishamco removed the request for review from jtkech January 15, 2024 21:09
@MikeAlhayek
Copy link
Member

@hishamco it feels wrong to me to override the service provider factory directly in OC. The approach I took in the other PR does not override anything but extend the feature. Either way, I don't think this problem is something we need to address in OC. It seems to be on the agenda for .Net 9. There is no pressing need to implement this so why not wait until it's part of the framework.

Maybe others will have other thoughts on this approach.

@hishamco
Copy link
Member Author

it feels wrong to me to override the service provider factory directly in OC.

Please check the DefaultServiceProviderFactory source

@sebastienros your thought if you have time

@hishamco hishamco mentioned this pull request Jan 21, 2024
@Piedone
Copy link
Member

Piedone commented Mar 22, 2024

Can you point to some .NET 9 issues or something, @MikeAlhayek? I can't find anything.

@MikeAlhayek
Copy link
Member

dotnet/runtime#95476

@Piedone
Copy link
Member

Piedone commented Mar 22, 2024

I see. So, to summarize:

Do I understand it correctly?

@MikeAlhayek
Copy link
Member

Yes @Piedone the summary is correct. And Both PR's should be closed since we don't really have to this in OC

@Piedone
Copy link
Member

Piedone commented Mar 22, 2024

OK then!

@Piedone Piedone closed this Mar 22, 2024
@hishamco hishamco deleted the hishamco/keyed-dictionary branch March 23, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants