Skip to content

Conversation

werkz-by-drake
Copy link

In the case where a TomlField is being applied to a Dictionary with enum keys, there is a possibility for an exception to be thrown that can't be handled well if an invalid enum key exists in the TOML data.

Reproduction:

(with IgnoreInvalidEnumValues set to true)

a. Parse a TOML string with multiple invalid keys
b. Parse a TOML string with one invalid key, and at least one other valid key at the default value

Result:

a. An key is added at the default (0) value with the invalid enum's value, which is hard to detect
b. An exception is thrown when a valid key at the default (0) value also exists
c. An exception is thrown when multiple invalid keys are found

Expected:

Invalid keys should not be added to the dictionary, multiple keys should never happen


Changes in this PR:

  • Invalid keys are no longer added to the resulting table.
  • Unit tests have also been added to ensure the new behaviour works, and that it doesn't break the behaviour of multiple keys being illegal under the TOML standard.

Previously, invalid enum keys would often crash the program when trying
to add the same key twice.

However, it may not be ideal for us to even add invalid keys to a
Dictionary; so I've provided an option that allows the user to skip
these fields from being added entirely.

In the case of Ignore, it will still set it to the first enum value as
it did before - but it will no longer crash when trying to add the same
key multiple times (in the case where multiple invalid keys were found,
or a valid key at the first enumerator also exists)
@coveralls
Copy link

Pull Request Test Coverage Report for Build 17082857410

Details

  • 17 of 17 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 91.199%

Totals Coverage Status
Change from base Build 15096414680: 0.3%
Covered Lines: 1948
Relevant Lines: 2136

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants