-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Option to throw exception when duplicate fields in section #34
Conversation
b7fcbfa
to
6597b6d
Compare
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.
Thanks for your contribution and sorry for the late reply / review! Good work there, also thanks for adding positive and negative unit tests 👍
I just have a few minor change requests, if you do not agree with them, I am happy to discuss 😃
include/inicpp.h
Outdated
@@ -419,6 +419,7 @@ namespace ini | |||
char esc_ = '\\'; | |||
std::vector<std::string> commentPrefixes_ = { "#" , ";" }; | |||
bool multiLineValues_ = false; | |||
bool exceptionWhenDuplicateField_ = false; |
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.
Generally I prefer positive formulation, so here you could rename the member to something like overwriteDuplicateFields_ = true
include/inicpp.h
Outdated
/** Sets whether or not to throw an exception if duplicate fields are found in a section. | ||
* Default is false. | ||
* @param enable enable or disable? */ | ||
void setExceptionWhenDuplicateField(bool enable) |
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.
Rename the function to something like allowOverwriteDuplicateFields
. Also Update the docs accordingly, i.e. it should say that if overwriting duplicate fields is not allowed, then the parser will throw an exception.
include/inicpp.h
Outdated
@@ -669,6 +678,13 @@ namespace ini | |||
// retrieve field name and value | |||
std::string name = line.substr(0, pos); | |||
trim(name); | |||
if (exceptionWhenDuplicateField_ && currentSection->count(name)) |
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.
Please add an explicit comparison, i.e. currentSection->count(name) > 0
or currentSection->count(name) != 0
425fbc0
to
91293f7
Compare
Thanks for adapting and your contribution 👍 |
Summary
This pull request adds the option to throw an exception when a duplicate field is found inside a section.
Problem
We need the option to detect if duplicate fields appear inside a section.
However, at the moment it is hard to detect duplicate fields inside a section after it has been decoded.
Solution
Similar to the
multiLineValues_
option, the pull request addsexceptionWhenDuplicateField_
. To avoid compatibility issues with prior versions, the option is disabled by default. It can be enabled and disabled with a call tosetExceptionWhenDuplicateField
. When enabled, an exception is thrown if a duplicate field is found inside a section. When disabled, no exception is thrown when duplicate fields are encountered and the previously decoded value is overwritten, which is the same behaviour as before this pull request.To ensure correctness, two test cases have been added to test setting the option to enabled and disabled. The default case is already covered by an existing test case.