Simplified programmatic configuration for bootstrap providers #19

[yevhen]

No description provided.

[dnfclas]

Hi @yevhen, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

[yevhen]

I've also added utility extension method for IDictionary lookup. It's internal. I think there many places in code where it could be useful. Finding smth in dictionary is so common :)

[veikkoeeva]

A slip?

[yevhen]

Probably

[yevhen]

The build has failed on Jenkins and it has nothing to do with my changes. The test for observers has failed for some intermittent reasons (timeouts).

[gabikliot]

We definitely should not limit to only one impl. of a given Bootstrap Provider. One should be able to pass multiple providers of the exact same type. We currently allow that. The difference in this PR is in the line below:
var config = new ProviderConfiguration( properties ?? new Dictionary<string, string>(), fullName, fullName);
the last arg should be provider name, not fullName of the type.
It means the RegisterBootstrapProvider should accept a name argument.

[yevhen]

Ok. I'll add name parameter

[gabikliot]

Yevgeny, you can also look at some examples here: #201
It's quite similar to your code, with small differences.
Your PR is basically putting the same code in the framework, instead of keeping it as an example, which is a nice and welcomed extension.

[gabikliot]

I also tried to run the tests. They indeed fail. The problem is that in your PR you removed the line: Providers = new Dictionary<string, IProviderConfiguration>(); in the ProviderCategoryConfiguration.Load.

It now puts a burden of making sure to init this property on the caller, and there were paths in the code that did not do that. I think it would be more robust to init it in the constructor.
In general I also don't like much the "explicit init" style of code
(category = new ProviderCategoryConfiguration()
731 + {
732 + Name = bootstrapCategoryKey,
733 + Providers = new Dictionary<string, IProviderConfiguration>()
734 + };

is it going back to C from C++? That's one of the pillars of OOP - encapsulation, right?

Anyway, it is also very easy to run the tests, either in the VS or by the Test script.

[yevhen]

@gabikliot

Your PR is basically putting the same code in the framework, instead of keeping it as an example, which is a nice and welcomed extension.

I didn't get it. Did you mean it's better to leave it as just an example and not try to add it to the framework?

[yevhen]

@gabikliot

I also tried to run the tests. They indeed fail. The problem is that in your PR you removed the line: Providers = new Dictionary(); in the ProviderCategoryConfiguration.Load.

That's a bug. I'll fix it.
Strange, I did run tests via cmd line as well. Perhaps, I haven't rebuild the code before running test.cmd.

[gabikliot]

I didn't get it. Did you mean it's better to leave it as just an example and not try to add it to the framework?

No, the other way. I liked it. I said it's better then the example approach I used in the sample.

[yevhen]

@gabikliot

(category = new ProviderCategoryConfiguration()
731 + {
732 + Name = bootstrapCategoryKey,
733 + Providers = new Dictionary()
734 + };

As per your example (gabikliot@844e26b). And follows the usual idiom when working with non-immutable objects.