-
Notifications
You must be signed in to change notification settings - Fork 311
Conversation
Hi @pakrym, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
|
||
namespace Microsoft.AspNet.Hosting | ||
{ | ||
public class WebHostConfiguration |
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.
Perhaps consider making this a POCO and just using ConfigurationBinder to directly bind against config?
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.
@HaoK can it handle two keys for one property and 1/0 boolean property value?
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.
Yeah, this isn't a pattern we use anywhere else.
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.
You could just have a string property that maps to the key in config, and then expose the current logic just like you do now.
public string DetailedErrors { get; set; }
public bool IsDetailed { get { <same logic as you have> }
🆙 📅 |
private const string ServerKey = "server"; | ||
private const string WebRootKey = "webroot"; | ||
|
||
public WebHostOptions(string application, bool detailedErrors, string environment, string server, string webRoot) |
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.
no params needed
🆙 📅 |
private const string ApplicationKey = "app"; | ||
private const string DetailedErrorsKey = "detailederrors"; | ||
private const string EnvironmentKey = "environment"; | ||
private const string OldEnvironmentKey = "ENV"; |
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.
nit:can you move this to the bottom
Nuke those tests that change the env and |
|| string.Equals("1", configuration[DetailedErrorsKey], StringComparison.OrdinalIgnoreCase); | ||
Environment = configuration[EnvironmentKey] ?? configuration[OldEnvironmentKey]; | ||
Server = configuration[ServerKey]; | ||
WebRoot = configuration[WebRootKey]; |
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.
So if you wanted to use the config binder, it would look something more like this, the only weirdness is with the Environment/DetailedErrors properties which need a method to add the logic.
public class WebHostOptions {
public string App { get; set; }
public string Server {get; set; }
public string WebRoot {get; set; }
public string DetailedErrors {get; set; }
public string Environment {get; set; }
public string Env {get; set; }
public bool ShowDetailedErrors { get {
return string.Equals("true", DetailedErrors)
|| string.Equals("1", DetailedErrors) ;
} }
public string ResolveEnvironment() {
return Environment ?? Env;
}
}
To get it out of config assuming its at the root level:
config.Get<WebHostOptions>();
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.
The upside, is you can nuke the keys (the property names for the POCO are automatically the keys. It would just work except for the weirdness around DetailedErrors and supporting the old Env key...
a2b9789
to
660f1ca
Compare
@pakrym make an announcement |
#453
@davidfowl @Tratcher @lodejard @DamianEdwards