SamboyCoding / Tomlet

Zero-Dependency, model-based TOML De/Serializer for .NET
MIT License
155 stars 29 forks source link

Quality of Life options for TomlTable and TomlValue #28

Closed johnkiddjr closed 1 year ago

johnkiddjr commented 1 year ago

Fixes #27

This adds the functionality of IEnumerator to TomlTable, allowing you to iterate on the table without needing to reference the Entries property.

foreach (var item in table.Entries)
{
    ...
}

Becomes

foreach (var item in table)
{
    ...
}

This also adds an override on the ToString method for TomlValue to return the StringValue property.

TomlString stringValue; //or any other TomlValue
Console.WriteLine(stringValue.StringValue);

Becomes

TomlString stringValue; //or any other TomlValue
Console.WriteLine(stringValue.ToString()); // outputs the value of StringValue now
coveralls commented 1 year ago

Coverage Status

coverage: 92.402% (-0.07%) from 92.47% when pulling d485db0c1f2799b4713d3a2cd1c3a6e0b9daffdf on johnkiddjr:master into acc83e19e7929b4760c7aa003b8902e82f70f090 on SamboyCoding:master.

SamboyCoding commented 1 year ago

I have no issues with the actual content of this pull request, so on that front, this is approved, but as per coveralls' comment above, the lack of unit tests for the enumeration and string values has resulted in coverage decreasing, which I'd rather avoid, if you wouldn't mind writing a couple tests to cover those cases -- though they really don't need it, it's easier to just enforce the rule of "test as high a percentage of the codebase as possible" rather than making case-by-case decisions.

johnkiddjr commented 1 year ago

Agreed, added tests to ensure ToString and StringValue are equal (done for both TomlString and TomlLong) and tests to check that iterated values are the same as values pulled directly from the dictionary.

SamboyCoding commented 1 year ago

Appreciate it! Assuming this CI run passes fine, I'll merge this then.

SamboyCoding commented 1 year ago

Right well the explicit interface implementation wasn't covered but I can live with that, merging.