Skip to content

Preserve Color.Empty when serializing - #1103

Merged
AArnott merged 3 commits into
v1.4from
aarnott-urban-garbanzo
Oct 7, 2026
Merged

AArnott merged 3 commits into
v1.4from
aarnott-urban-garbanzo

Conversation

@AArnott

@AArnott AArnott commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner

Why

The built-in System.Drawing.Color converter loses Color.Empty identity because it only stores ARGB, causing the empty color to deserialize as transparent black.

What changed

Encode Color.Empty as the MessagePack boolean false while retaining ARGB integer encoding for non-empty colors. Deserialization accepts false and existing integer payloads, and rejects true. The schema and docs describe the integer-or-false representation. Tests cover both Color.Empty and transparent black.

Validation

  • Focused TUnit tests passed on .NET 8 and .NET 10.
  • DocFX build passed.

Closes #1102

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 7, 2026 13:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Nullable empty colors lose their identity, and nullable-color array schemas reject valid null elements.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Updates the built-in color converter to distinguish Color.Empty from transparent black, addressing #1102.

Changes:

  • Encodes empty colors as nil while retaining ARGB integers for non-empty colors.
  • Updates the schema and documentation.
  • Adds round-trip tests for empty colors and transparent black.
File Description
test/​Nerdbank.MessagePack.TUnit/​BuiltInConverterTests.cs Tests color identity and encoding.
src/​Nerdbank.MessagePack/​Converters/​BclConverters.cs Adds nil handling and updates the color schema.
docfx/​docs/​built-in-converters.md Documents the nil-or-integer representation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Nerdbank.MessagePack/Converters/BclConverters.cs Outdated
Comment thread src/Nerdbank.MessagePack/Converters/BclConverters.cs
Use a boolean marker for Color.Empty so nullable null remains nil, and cover nullable color schemas across dialects.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 7, 2026 13:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation preserves nullable semantics and integer compatibility, with targeted regression coverage and no blocking issues.

Review effort: Balanced
Findings: None

Resolved since last review (2)

Make nullable schema composition tolerate overlapping null branches and record the generated Color schemas for both dialects.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 7, 2026 14:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The encoding, nullable handling, schemas, and regression coverage are consistent, with no blocking issues identified.

Review effort: Balanced
Findings: None

@AArnott
AArnott enabled auto-merge (squash) October 7, 2026 14:11
@codecov

codecov Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.59459% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.83%. Comparing base (7ea3507) to head (289fce6).

Files with missing lines Patch % Lines
...c/Nerdbank.MessagePack/Converters/BclConverters.cs 84.61% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             v1.4    #1103      +/-   ##
==========================================
+ Coverage   82.80%   82.83%   +0.02%     
==========================================
  Files         259      259              
  Lines       21296    21327      +31     
  Branches     2810     2814       +4     
==========================================
+ Hits        17634    17666      +32     
+ Misses       2762     2760       -2     
- Partials      900      901       +1     
Flag Coverage Δ
Linux 82.30% <94.59%> (-0.09%) ⬇️
Windows 82.55% <94.59%> (+0.04%) ⬆️
macOS 82.29% <94.59%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AArnott
AArnott merged commit f09460f into v1.4 Oct 7, 2026
10 checks passed
@AArnott
AArnott deleted the aarnott-urban-garbanzo branch October 7, 2026 14:28
@AArnott AArnott added this to the 1.4 milestone Oct 7, 2026
@AArnott AArnott mentioned this pull request Oct 10, 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