refactor ini file handling logic for write command #454
Conversation
cfbao
force-pushed
the
cbao/write-refactor
branch
from
cd0a88e to
49d3aba
Compare
| var data = parser.ReadFile( output ); | ||
| string awsConfigFilePath = useCredentialProcess ? output : SharedCredentialsFile.DefaultConfigFilePath; | ||
| var awsConfig = GetParsedIniFileOrNull( awsConfigFilePath ); | ||
|
|
||
| string awsCredentialsFilePath = useCredentialProcess ? SharedCredentialsFile.DefaultFilePath : output; | ||
| var awsCredentials = GetParsedIniFileOrNull( awsCredentialsFilePath ); |
There was a problem hiding this comment.
This is the main change.
My main pet peeve in this area is that output and data could have one of two different meanings, depending on the value of useCredentialProcess. I find this confusing lower down where the logic splits into two branches.
So, here, I changed the variable assignment so that output will no longer be used beyond this point, and we will have distinct names for the two different types of files/data.
As a nice side-effect, this is also the last place where we do any INI file parsing, or have any reference to SharedCredentialsFile.
| parser.WriteFile( SharedCredentialsFile.DefaultConfigFilePath, defaultConfigFile ); | ||
| parser.WriteFile( awsConfigFilePath, awsConfig, Utf8 ); |
There was a problem hiding this comment.
nit: without explicitly specifying the encoding, parser will write file using UTF8-BOM 🤢
cfbao
requested review from
boarnoah,
scowing,
jharoutuniand2l,
jkomonen and
gord5500
| string sectionName = $"profile {profile}"; | ||
| if( !data.Sections.ContainsSection( sectionName ) ) { | ||
| data.Sections.AddSection( sectionName ); | ||
| Debug.Assert( awsConfig is not null ); |
There was a problem hiding this comment.
What is the thought process behind this being an assertion only in debug builds?
EDIT: Also we got BMX_DEBUG, should it just be Assert in all builds that is only tested with BMX_DEBUG = 1 ?
There was a problem hiding this comment.
The main goal here is to make the compiler's nullable analysis happy in a clear and intentional way.
awsConfig should not be null here when useCredentialProcess is true (because we'd create the AWS config file above). The compiler cannot infer this, so by default it'd mark awsConfig as "may be null", and issue warnings at L88 and below.
We could use the null-forgiving operator !, but it doesn't make our assumptions and intent clear:
- Why can we assume
awsConfigisn't null in these places? - When does this assumption hold?
By using Debug.Assert, we're effectively making clarifying statements like
fooisn't null inside this if/else/for/while blockbarisn't null once this above statement is executed successfully
We can also avoid sprinkling ! in many different places.
The fact that this is also a runtime assertion in debug builds is more or less a side effect that I don't care that much. I don't think we should make it actually asserts at runtime when BMX_DEBUG is set either.
There was a problem hiding this comment.
We've used this technique to document our assumptions in code before, e.g.
It's also used by the .NET runtime libraries, e.g.
These are assumptions that should always hold true. If there's a real non-negligible chance that they could be false, then they should become real runtime assertions like
if ( !assumption ) {
throw new InvalidOperationException( "bad condition" );
}
follow up to this comment: #453 (comment)