Environment variables

[federico-paolillo]

This PR introduces support to environment variables as fallback values for command options.

The interface IEnvironmentVariablesProvider has been introduced to abstract away environment variables access. Given the very general interface methods, this interface has the side effect of allowing variables to come from anywhere (e.g.: a file, Redis, a database), enabling flexibility in what can be considered an environment by an implementor.

I decided to avoid putting all system variables on CommandInput and instead I decided to acces environment variables only on demand and when the name is known.
Using CommandInput would have required to put all the env. variables on and instance of CommandInput because the actual environment variable names required are known only on the CommandSchema and every CommandOptionSchema. Additionally exposing all the environment variables of a system can be a security risk as they may contain secrets that could be leaked in logging.

To access environment variables by name a CommandOptionSchema instance is needed in oreder to read the property EnvironmentVariableName which, as the name implies, contains the needed variable name.
I decided to extend CommandInitializer by "inverting" the foreach loop: insteand of going through every CommandOptionInput the loop now goes through every CommandOptionSchema and in case a given schema does not have a CommandOptionInput environment variables are queried for values. Once values are found they are turned into a substitutive CommandOptionInput and processed as always.

To allow flexibility and guarantee testability I've also extended IApplicationBuilder to let an user specify a custom IEnvironmentVariablesProvider.

There a few open points before considering this PR complete, in particular:

• What to document on README about this feature
• How help text should look like for options with env. variables support
• SecurityExceptions raised when accessing variables are ignored for now

Closes #3

[codecov[bot]]

Codecov Report

Merging #27 into master will decrease coverage by 0.66%. The diff coverage is 89.79%.

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
- Coverage   97.91%   97.25%   -0.67%     
==========================================
  Files          29       29              
  Lines         768      800      +32     
==========================================
+ Hits          752      778      +26     
- Misses         16       22       +6

Impacted Files	Coverage Δ
CliFx/CliApplicationBuilder.cs	98.14% <100%> (+0.1%)	:arrow_up:
CliFx/Models/CommandInput.cs	100% <100%> (ø)	:arrow_up:
CliFx/Attributes/CommandOptionAttribute.cs	100% <100%> (ø)	:arrow_up:
CliFx/Models/Extensions.cs	100% <100%> (ø)	:arrow_up:
CliFx/Models/CommandOptionSchema.cs	100% <100%> (ø)	:arrow_up:
CliFx/Services/CommandSchemaResolver.cs	100% <100%> (ø)	:arrow_up:
CliFx/Services/Extensions.cs	90.9% <50%> (-9.1%)	:arrow_down:
CliFx/Services/CommandInputParser.cs	94.73% <75%> (-5.27%)	:arrow_down:
CliFx/Services/CommandInitializer.cs	92.59% <91.66%> (-7.41%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 25cbfdb...3130251. Read the comment docs.

[federico-paolillo]

I believe CodeCov complains about missing coverage in EnvironmentVariablesProvider.
I did not test that class because it would be a very hefty integration test that would require setting environment variables in the test system.

I'm not really sure how to proceed, is it possible to ignore code coverage for specific files ?

[Tyrrrz]

Don't worry regarding coverage. It looks like you dropped 7% in CommandInitializer but there are only 3 uncovered statements (constructor overload).

[federico-paolillo]

I'll get to work in the next days to fix all the requested changes and will report back.

I cannot really argue that env. are more than enough for a console application so the additional flexibility might be wasted (apart for testing). Should i try to rewrite the logic to use CommandInput instead of IEnvironmentVariablesProvider and see how it looks ?

[Tyrrrz]

I'll get to work in the next days to fix all the requested changes and will report back.

Awesome, thanks :)

I cannot really argue that env. are more than enough for a console application so the additional flexibility might be wasted (apart for testing). Should i try to rewrite the logic to use CommandInput instead of IEnvironmentVariablesProvider and see how it looks?

If you have time, that would be nice. If not, I'll just tackle this later.

[federico-paolillo]

Finally got time to fix everything.

As dicussed environment variables are now provided via CommandInput.EnvironmentVariables, to get variables CommandInputParser still calls IEnvironmentVariablesProvider. This is needed to make the implementation testable, calling Environment.GetEnvironmentVariables directly would make impossible to test CommandInputParser.

I've introduced a new class called EnvironmentVariablesParser that is responsible to process an environment variable value and parse it so that it matches the criteria discusses before (e.g.: split values only when an option is a collection, etc...). This class is invoked by CommandInitializer because in order to parse variables correctly we must know for what OptionSchema we must provide values.

I've decided that in case a SecurityException is raised it is simply swallowed, rethrowing the exception wrappend in a CliFxException is not suitable because it would force applications that do not use environment variables to have access to them because CommandInputParser does not know in advance if an option requires variables or not and will access variables anyway.

EmptyEnviromentVariablesStub has been introduced to avoid creating many constructor overloads for CommandInput and to keep testing code as close to before as possible.

Escaping variables is not supported at the moment.

[Tyrrrz]

Thank you for your effort! I'll merge it now and make a few cosmetic changes later so you don't have to spend your time on this. :)