Fix TODOs, typos, and add missing tests#1086
Merged
EdwardCooke merged 4 commits intoaaubry:masterfrom Apr 9, 2026
Merged
Conversation
- Fix 'whete' -> 'where' in IObjectDescriptor.cs - Fix 'cannot be maped' -> 'cannot be mapped' in DeserializerBuilder, SerializerBuilder, StaticDeserializerBuilder, StaticSerializerBuilder (6 occurrences across 4 files) - Fix 'serializer complext objects' -> 'serialize complex objects' in DateTimeConverter, DateTimeOffsetConverter, DateTime8601Converter, TimeOnlyConverter (4 occurrences across 4 files) Total: 10 typo fixes across 9 files.
- Clarify DateTime parsing comment: DateTime.Parse with InvariantCulture is appropriate when the target type is explicitly DateTime. - Clarify binary/octal numberFormat comment: these formats are inherently locale-independent and Convert.ToUInt64 does not accept IFormatProvider. - Implement base-60 (sexagesimal) digit validation: chunks after the first must be in range [0, 59]. Throws FormatException for invalid values. - Add comprehensive tests for DateTime parsing, base-60 integer validation, and various integer base formats (binary, octal, hex).
The required-properties enforcement in ObjectNodeDeserializer was already fully implemented: it checks property.Required and throws YamlException listing missing property names. The two TODO comments were leftover markers from when the feature was first scaffolded. Remove the stale TODO comments and add tests for: - Both required properties missing (error lists both names) - Descriptive error message format - Non-enforcement mode does not throw for missing required members
The DeserializeScalarLongBase60Number test used '6_2' (=62) as a sexagesimal digit, which is invalid per the new validation that digits after the first must be < 60. Changed to '5_2' (=52) and updated the expected result accordingly.
Contributor
Author
|
The CI was failing due to a conflict between the new sexagesimal validation added in this PR and a pre-existing test. Root cause: Fix: Changed the test input to |
EdwardCooke
approved these changes
Apr 9, 2026
This was referenced Apr 9, 2026
This was referenced Apr 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR resolves stale TODO comments, fixes typos in user-facing messages, and adds missing test coverage across three areas.
1. Fix typos in error and documentation messages
Multiple user-facing error messages and XML documentation comments contain typos:
YamlDotNet/Serialization/IObjectDescriptor.cswhete→whereYamlDotNet/Serialization/DeserializerBuilder.csmaped→mapped(×2)YamlDotNet/Serialization/SerializerBuilder.csmaped→mappedYamlDotNet/Serialization/StaticDeserializerBuilder.csmaped→mapped(×2)YamlDotNet/Serialization/StaticSerializerBuilder.csmaped→mappedYamlDotNet/Serialization/Converters/DateTimeConverter.csserializer complext→serialize complexYamlDotNet/Serialization/Converters/DateTimeOffsetConverter.csserializer complext→serialize complexYamlDotNet/Serialization/Converters/DateTime8601Converter.csserializer complext→serialize complexYamlDotNet/Serialization/Converters/TimeOnlyConverter.csserializer complext→serialize complex2. Resolve TODO comments in ScalarNodeDeserializer
ScalarNodeDeserializer.cscontained three TODO comments marking incomplete or questionable logic:DateTime.ParsewithInvariantCultureandRoundtripKindis correct when the target type is explicitlyDateTime.Convert.ToUInt64does not accept anIFormatProviderfor these bases.1:99now throw aFormatException.Tests added:
DateTime_ParsesValidValues,DateTime_ThrowsOnInvalidValues,Base60Integer_ParsesValidValues,Base60Integer_RejectsInvalidSexagesimalDigit,Base60Integer_AllowsFirstChunkAbove59,Base60Integer_RejectsSecondChunkOf60,IntegerBases_ParseCorrectly3. Remove stale TODO comments from required-properties enforcement
ObjectNodeDeserializer.cscontained two stale TODO comments. The implementation was already complete (checksproperty.Requiredand throwsYamlExceptionlisting missing property names). Removed the leftover markers.Tests added:
WithRequiredMemberSet_ThrowsWhenBothMissing_ListsBothNames,WithRequiredMemberSet_ThrowsWithDescriptiveMessage,WithoutEnforceRequiredMembers_DoesNotThrowWhenMissing