Skip to content

Fix TODOs, typos, and add missing tests#1086

Merged
EdwardCooke merged 4 commits intoaaubry:masterfrom
fdcastel:fix-todos-typos
Apr 9, 2026
Merged

Fix TODOs, typos, and add missing tests#1086
EdwardCooke merged 4 commits intoaaubry:masterfrom
fdcastel:fix-todos-typos

Conversation

@fdcastel
Copy link
Copy Markdown
Contributor

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:

File Typo Fixed
YamlDotNet/Serialization/IObjectDescriptor.cs whetewhere
YamlDotNet/Serialization/DeserializerBuilder.cs mapedmapped (×2)
YamlDotNet/Serialization/SerializerBuilder.cs mapedmapped
YamlDotNet/Serialization/StaticDeserializerBuilder.cs mapedmapped (×2)
YamlDotNet/Serialization/StaticSerializerBuilder.cs mapedmapped
YamlDotNet/Serialization/Converters/DateTimeConverter.cs serializer complextserialize complex
YamlDotNet/Serialization/Converters/DateTimeOffsetConverter.cs serializer complextserialize complex
YamlDotNet/Serialization/Converters/DateTime8601Converter.cs serializer complextserialize complex
YamlDotNet/Serialization/Converters/TimeOnlyConverter.cs serializer complextserialize complex

2. Resolve TODO comments in ScalarNodeDeserializer

ScalarNodeDeserializer.cs contained three TODO comments marking incomplete or questionable logic:

  1. DateTime parsing TODO: Replaced misleading "probably incorrect" comment with a clarifying note — DateTime.Parse with InvariantCulture and RoundtripKind is correct when the target type is explicitly DateTime.
  2. Binary/octal numberFormat TODO: Replaced with a comment explaining that binary/octal parsing is inherently locale-independent; Convert.ToUInt64 does not accept an IFormatProvider for these bases.
  3. Base-60 validation TODO: Implemented actual digit validation — chunks after the first in sexagesimal notation must be in range [0, 59]. Values like 1:99 now throw a FormatException.

Tests added: DateTime_ParsesValidValues, DateTime_ThrowsOnInvalidValues, Base60Integer_ParsesValidValues, Base60Integer_RejectsInvalidSexagesimalDigit, Base60Integer_AllowsFirstChunkAbove59, Base60Integer_RejectsSecondChunkOf60, IntegerBases_ParseCorrectly

3. Remove stale TODO comments from required-properties enforcement

ObjectNodeDeserializer.cs contained two stale TODO comments. The implementation was already complete (checks property.Required and throws YamlException listing missing property names). Removed the leftover markers.

Tests added: WithRequiredMemberSet_ThrowsWhenBothMissing_ListsBothNames, WithRequiredMemberSet_ThrowsWithDescriptiveMessage, WithoutEnforceRequiredMembers_DoesNotThrowWhenMissing

- 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.
@fdcastel
Copy link
Copy Markdown
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: ScalarNodeDeserializer.DeserializeIntegerHelper was updated to enforce that sexagesimal (base-60) digits after the first must be in the range [0, 59]. The existing DeserializeScalarLongBase60Number test used the value "99_:_58:47:3:6_2:10", where 6_2 (= 62) is not a valid base-60 digit, so the new validation correctly rejected it.

Fix: Changed the test input to "99_:_58:47:3:5_2:10" (using 5_2 = 52, which is a valid base-60 digit) and updated the expected result from 77744246530L to 77744245930L.

@EdwardCooke EdwardCooke merged commit b759707 into aaubry:master Apr 9, 2026
1 check passed
This was referenced Apr 9, 2026
This was referenced Apr 13, 2026
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