add LibraryName at ConfigurationOptions level #2502
Conversation
|
Question here: how important is the connection string version? IMO we can always add it later, and I'd like to avoid the commas problem, which I can easily see someone hitting here - thoughts? |
|
@NickCraver I'd be fine with dropping it and making it a code-only feature |
|
If cool with that, IMO better play and maybe it never comes up...don't want to have to deal with parsing if we can just play coy :) |
| @@ -49,10 +49,12 @@ public static void AddProvider(DefaultOptionsProvider provider) | |||
| /// </summary> | |||
| public virtual bool IsMatch(EndPoint endpoint) => false; | |||
|
|
|||
| private static readonly DefaultOptionsProvider s_DefaultProvider = new DefaultOptionsProvider(); | |||
There was a problem hiding this comment.
I'm a little uncomfortable here because there's a chance this becomes mutable at some point down the road (and we're not returning a new one each time). Thoughts?
There was a problem hiding this comment.
@NickCraver the AddProvider API already kinda assumes that it isn't, since that is instance based; so if it comes up as a mutability issue: we already have a bigger problem
There was a problem hiding this comment.
@NickCraver for example, today you could just do DefaultOptionsProvider.AddProvider(new DefaultOptionsProvider()); - and that needs to work
There was a problem hiding this comment.
I agree that'd happen if you did that due to matching, but today every caller would be getting their own instance, no? I'm not sure how shifting this changes the example, but does return the same instance for all.
I'm saying this because clientName is stateful, and has the potential to change on subsequent caller/connections as the environment does (e.g. restarting your Azure app after an environment change isn't uncommon).
There was a problem hiding this comment.
Then any subclass already has this problem?
There was a problem hiding this comment.
hmm, yeah it could indeed, good point - this just makes it worse for the default Azure case.
There was a problem hiding this comment.
I'm happy to revert the default instance for now and take that point separately; this isn't a hill for today
There was a problem hiding this comment.
If we can please - I know this will break some scenarios (silently reporting old IDs, few would notice)
|
pushed changes to revert |
context: desire to set library name more granularly than provider - for example output cache provider, distributed cache provider, etc
the plan is also to, where possible, respect the pre-existing lib name - i.e. if the provider says "foostore", we'll use the new
GetProviderAPI, ask for that info, and specifiy "foostore ocache", "foostore dcache", etc