Tyrrrz / CliFx

Class-first framework for building command-line interfaces
MIT License
1.48k stars 60 forks source link

Fix application crashes if there are two environment variables with same name, differing only in case #67

Closed ron-myers closed 4 years ago

ron-myers commented 4 years ago

Summary

This corrects the bug identified in #64.

Approach

The solution to this was less obvious than I expected when I sat down to build this.

As I saw it, there were a few options here:

I then read:

https://docs.microsoft.com/en-us/dotnet/api/system.environment.getenvironmentvariable

which includes:

To retrieve all environment variables along with their values, call the GetEnvironmentVariables method. 
Environment variable names are case-sensitive on Linux and macOS but are not case-sensitive on Windows.

Then attempted to set two environment variables on Windows that only differed by case and did not find a way to do so.

So decided to approach with Ordinal string comparison and submit for discussion.

Going with this approach required a change in CommandOptionsSchema

Added a unit test to verify the behaviour.

Happy to iterate if you have a preferred approach.

Fixes #64

Tyrrrz commented 4 years ago

Hi, thanks for PR!

I guess the name of test Environment_variables_pulled_ignoring_case is wrong, it should say "while_respecting_case", right?

Please also add a comment about case sensitivity on different OSes to CliApplication.RunAsync where the environment variables are extracted. It would be useful to know in the future why this decision was made.

Tyrrrz commented 4 years ago

Also, this part will need to be updated as well:

https://github.com/Tyrrrz/CliFx/blob/b4918187796a85a2a925f11c2292d6004ef2c1cc/CliFx.Analyzers/CommandSchemaAnalyzer.cs#L207-L212

Tyrrrz commented 4 years ago

Thanks!