#2229 A little refactoring, add new class for interpretting options #2273
Conversation
…ronmentVariableGetter and OptionsInterpretter classes
| { | ||
| if (environmentVariableGetter == null) | ||
| { | ||
| throw new ArgumentException("Required parameter", nameof(environmentVariableGetter)); |
There was a problem hiding this comment.
ArgumentException [](start = 26, length = 17)
Change to argumentNullException. With that, u can you just the nameof(xyz) #Resolved
There was a problem hiding this comment.
you could also do this: _environmentVariableGetter = environmentVariableGetter ?? throw new ArgumentNullException(nameof(enviromentVariableGetter));
In reply to: 566070993 [](ancestors = 566070993)
There was a problem hiding this comment.
| analyzeOptionsBase.Level = analyzeOptionsBase.Level.Union(levelsFromEnv); | ||
| } | ||
|
|
||
| private IEnumerable<T> getOptionEnumFromEnvVar<T>(string propertyName) |
There was a problem hiding this comment.
getOptionEnumFromEnvVar [](start = 31, length = 23)
GetOptionEnumFromEnvVar #Resolved
There was a problem hiding this comment.
|
|
||
| private IEnumerable<T> getOptionEnumFromEnvVar<T>(string propertyName) | ||
| { | ||
| return _environmentVariableGetter.GetEnvironmentVariable(FormEnvironmentVariableName(propertyName))?.Split(';')?.Select(x => Enum.Parse(typeof(T), x, ignoreCase: true))?.ToList()?.Cast<T>(); |
There was a problem hiding this comment.
_environmentVariableGetter.GetEnvironmentVariable [](start = 19, length = 49)
lets wrap. too long to read in one line #Resolved
There was a problem hiding this comment.
| IEnumerable<FailureLevel> levelsFromEnv = getOptionEnumFromEnvVar<FailureLevel>(nameof(analyzeOptionsBase.Level)); | ||
|
|
||
| // Union in C# is distinct | ||
| analyzeOptionsBase.Kind = analyzeOptionsBase.Kind.Union(kindsFromEnv); |
There was a problem hiding this comment.
Union(kindsFromEnv); [](start = 62, length = 20)
do we need to check for null? #Resolved
There was a problem hiding this comment.
…/github.com/microsoft/sarif-sdk into users/v-jwinkler/#2229_EnvironmentVariables
|
This pull request introduces 2 alerts when merging 5d1e28e into 689b73d - view on LGTM.com new alerts:
|
Add unit tests for new OptionsInterpretter
Fixes #2229
Move analyzeTestOptions to same project as al other options. Add EnvironmentVariableGetter and OptionsInterpretter classes