Skip to content

Fix TODOs, typos, and add missing tests#1086

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

Fix TODOs, typos, and add missing tests#1086
EdwardCooke merged 4 commits into
aaubry: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
mkaraki pushed a commit to mkaraki/Diffcord that referenced this pull request Jul 21, 2026
Updated [Discord.Net](https://github.com/discord-net/Discord.Net) from
3.19.1 to 3.20.1.

<details>
<summary>Release notes</summary>

_Sourced from [Discord.Net's
releases](https://github.com/discord-net/Discord.Net/releases)._

## 3.20.1

## [3.20.1] - 2026-06-07
This release fixes a regression introduced in 3.20.0

### Fixed
- #​3276 Handle null VoiceChannel in SocketVoiceState constructor
(61ed916)

**Full Changelog**:
discord-net/Discord.Net@3.20.0...3.20.1

## 3.20.0

## [3.20.0] - 2026-06-06
This release brings support for checkboxes and checkbox/radio groups in
modals, and also covers the "new" message search endpoint.

### Breaking changes
- `SelectMenuOptionAttribute` from the Interaction Framework was renamed
to `EnumOptionAttribute`.

### Added
- #​3232 IF modal radio buttons, and checkboxes (c95fbf6)
- #​3268 add support for getting messages from a guild (with filters)
(31fed25)
- #​3255 add missing audit log action types (4476eea)
- #​3265 Add GET voice-state REST wrappers (13d83da)

### Fixed
- #​3258 propagate parent module attributes to child commands (cbc61d9)
- #​3263 strip RTP padding before DAVE decrypt (RFC 3550 В§5.1)
(1a843fb)
- #​3256 Add empty payload check (6527e71)
- #​3264 Fix reference to PreCompiledLambdas/UseCompiledLambda (763aa79)
- #​3271 fix for #​3269 (9abfbfd)
- #​3272 Fix default array converter in modals & add docs for
checkboxes/radio groups (527764c)

### Misc
- #​3254 user `global_name` description (05af64b)
- #​3257 feat(Core): add missing JSON error codes (4272ae1)
- #​3259 refactor(Core): rename JSON error code (504e1db)
- #​3261 Message call data timestamp nullability (5a328a0)
- #​3266 Add play audio sample (4d8b0bc)

## New Contributors
* @​Archivelit made their first contribution in
discord-net/Discord.Net#3256
* @​Sim-hu made their first contribution in
discord-net/Discord.Net#3255
* @​yury-opolev made their first contribution in
discord-net/Discord.Net#3263
* @​apartje made their first contribution in
discord-net/Discord.Net#3271

**Full Changelog**:
discord-net/Discord.Net@3.19.1...3.20.0

Commits viewable in [compare
view](discord-net/Discord.Net@3.19.1...3.20.1).
</details>

Updated [DotNetEnv](https://github.com/tonerdo/dotnet-env) from 3.1.1 to
3.2.0.

<details>
<summary>Release notes</summary>

_Sourced from [DotNetEnv's
releases](https://github.com/tonerdo/dotnet-env/releases)._

## 3.2.0

- Switch parsing to Superpower (from Sprache)
- Fix utf8 parsing
- Interpolated variables parsing


Commits viewable in [compare
view](tonerdo/dotnet-env@v3.1.1...v3.2.0).
</details>

Updated [YamlDotNet](https://github.com/aaubry/YamlDotNet) from 16.3.0
to 18.1.0.

<details>
<summary>Release notes</summary>

_Sourced from [YamlDotNet's
releases](https://github.com/aaubry/YamlDotNet/releases)._

## 18.1.0

## What's Changed
* Use NET 10 with benchmarks by @​mcraiha in
aaubry/YamlDotNet#1099
* Revert package upgrades by @​EdwardCooke in
aaubry/YamlDotNet#1104
* Added default maximum recursion level of 130 (max when using defaults
on Windows/.net8) by @​EdwardCooke in
aaubry/YamlDotNet#1110
* Static deserializer builder needed the default maximum recursion by
@​EdwardCooke in aaubry/YamlDotNet#1111

## New Contributors
* @​mcraiha made their first contribution in
aaubry/YamlDotNet#1099

**Full Changelog**:
aaubry/YamlDotNet@v18.0.0...v18.1.0

## Breaking
* Maximum depth of yaml files is now 130 by default. If you need higher
you will need to adjust the maximum yaml depth. Going above 130 runs the
risk of stack overflow exceptions when any exception happens inside of
the deserialization

## 18.0.0

## What's Changed
* Add a parse method wrapper and caching to fix AoT compilation by
@​EdwardCooke in aaubry/YamlDotNet#1103
**BREAKING CHANGE** This is a breaking change in the
`TypeInspectorSkeleton` class and the `ITypeInspector` interface by
adding 2 methods . Quick fix to resolve those breaking changes in your
own custom TypeInspector is to return false on the HasParseMethod method
and return null or throw an exception on the Parse method.


**Full Changelog**:
aaubry/YamlDotNet@v17.1.0...v18.0.0

## 17.1.0

## What's Changed
* Security improvements by @​EdwardCooke in
aaubry/YamlDotNet#1102
There was a potential breaking change for large yaml files in the
MergingParser. You may need to specify the optional parameter for
maximum events to be processed. It default to 100k events which is a
very large yaml file.


**Full Changelog**:
aaubry/YamlDotNet@v17.0.0...v17.1.0

## 17.0.0

## What's Changed
* Clean-up the "IsKey" logic by @​aaubry in
aaubry/YamlDotNet#1073
* Fix for gitversion and pinning it so it doesnt break...again. by
@​EdwardCooke in aaubry/YamlDotNet#1074
* Add max depth handling to StaticDeserializerBuilder (builds on #​1072)
by @​skdishansachin in aaubry/YamlDotNet#1082
* Allow specifying a maximum recursion for the deserializer by @​aaubry
in aaubry/YamlDotNet#1072
* Fix NullReferenceException when serializing null System.Type
properties by @​fdcastel in
aaubry/YamlDotNet#1091
* Reduce code duplication in converters and event emitters by @​fdcastel
in aaubry/YamlDotNet#1090
* Use pre-compiled static Regex instances in ScalarNodeDeserializer by
@​fdcastel in aaubry/YamlDotNet#1088
* Fix infinite loop in source generator exception handler by @​fdcastel
in aaubry/YamlDotNet#1087
* Fix TODOs, typos, and add missing tests by @​fdcastel in
aaubry/YamlDotNet#1086
* Fix YamlException.ToString() to include stack trace by
@​skdishansachin in aaubry/YamlDotNet#1084
* Fix remaining spec cases during parsing: L383, C2SP by @​am11 in
aaubry/YamlDotNet#1081
* Improve type fidelity in UnquotedStringTypeDeserialization test by
@​jhgbrt in aaubry/YamlDotNet#1076
* CodeQL Advanced Workflow by @​aluty in
aaubry/YamlDotNet#1067
* Nullable fixes in non-public code by @​Kielek in
aaubry/YamlDotNet#1064
* Use string interning by @​simonthum in
aaubry/YamlDotNet#1055
* Fix grammar in comments in DefaultValuesHandling.cs by @​209jkjkjk in
aaubry/YamlDotNet#1041
* fix #​1031 by @​dogdie233 in
aaubry/YamlDotNet#1033
* Improve Native AOT Support (Closes #​1085) by @​fdcastel in
aaubry/YamlDotNet#1092

## New Contributors
* @​skdishansachin made their first contribution in
aaubry/YamlDotNet#1082
* @​fdcastel made their first contribution in
aaubry/YamlDotNet#1091
* @​jhgbrt made their first contribution in
aaubry/YamlDotNet#1076
* @​aluty made their first contribution in
aaubry/YamlDotNet#1067
* @​Kielek made their first contribution in
aaubry/YamlDotNet#1064
* @​simonthum made their first contribution in
aaubry/YamlDotNet#1055
* @​209jkjkjk made their first contribution in
aaubry/YamlDotNet#1041
* @​dogdie233 made their first contribution in
aaubry/YamlDotNet#1033

**Full Changelog**:
aaubry/YamlDotNet@v16.3.0...v17.0.0

Commits viewable in [compare
view](aaubry/YamlDotNet@v16.3.0...v18.1.0).
</details>

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore <dependency name> major version` will close this
group update PR and stop Dependabot creating any more for the specific
dependency's major version (unless you unignore this specific
dependency's major version or upgrade to it yourself)
- `@dependabot ignore <dependency name> minor version` will close this
group update PR and stop Dependabot creating any more for the specific
dependency's minor version (unless you unignore this specific
dependency's minor version or upgrade to it yourself)
- `@dependabot ignore <dependency name>` will close this group update PR
and stop Dependabot creating any more for the specific dependency
(unless you unignore this specific dependency or upgrade to it yourself)
- `@dependabot unignore <dependency name>` will remove all of the ignore
conditions of the specified dependency
- `@dependabot unignore <dependency name> <ignore condition>` will
remove the ignore condition of the specified dependency and ignore
conditions


</details>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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