Add migrating-newtonsoft-to-system-text-json skill - #89
Conversation
…vement) Teaches migration from Newtonsoft.Json to System.Text.Json: attribute mapping differences, behavioral changes (casing, strictness, null handling), custom converter conversion, JToken->JsonElement/JsonNode migration, and polymorphic serialization with JsonDerivedType. Eval results: +13.8% improvement over baseline (threshold: 10%) Includes eval.yaml with migration scenario + negative test.
There was a problem hiding this comment.
Pull request overview
This PR adds a new skill for migrating from Newtonsoft.Json to System.Text.Json. The skill aims to help developers understand behavioral differences, attribute mappings, and common migration patterns. The PR includes both the skill documentation (SKILL.md) and evaluation scenarios (eval.yaml) with a reported 13.8% improvement in evaluation metrics.
Changes:
- Added
migrating-newtonsoft-to-system-text-jsonskill with comprehensive migration guidance - Added evaluation scenarios testing attribute migration and skill activation patterns
- Included behavioral differences table, attribute mappings, converter examples, and configuration guidance
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| src/dotnet/skills/migrating-newtonsoft-to-system-text-json/SKILL.md | Comprehensive migration guide covering behavioral differences, attribute mappings, custom converters, and configuration examples |
| src/dotnet/tests/migrating-newtonsoft-to-system-text-json/eval.yaml | Two evaluation scenarios: positive migration case and negative case to prevent false activation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| // Match Newtonsoft.Json default behavior: | ||
| options.PropertyNamingPolicy = JsonNamingPolicy.CamelCase; // Newtonsoft default | ||
| options.PropertyNameCaseInsensitive = true; // Newtonsoft default |
There was a problem hiding this comment.
The comment "Newtonsoft default" on line 68 is misleading. PropertyNameCaseInsensitive is not a Newtonsoft.Json default behavior. Newtonsoft.Json is case-sensitive by default during deserialization, just like System.Text.Json.
This comment should be removed or changed to clarify that this is an optional configuration, not matching a Newtonsoft default.
| - "Replaced NullValueHandling.Ignore with [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]" | ||
| - "Replaced [JsonConverter(typeof(StringEnumConverter))] with System.Text.Json equivalent (JsonStringEnumConverter)" | ||
| - "Changed [JsonExtensionData] Dictionary value type from JToken to JsonElement (critical difference!)" | ||
| - "Configured PropertyNameCaseInsensitive = true to match Newtonsoft default case-insensitive behavior" |
There was a problem hiding this comment.
The rubric item claims PropertyNameCaseInsensitive should be configured "to match Newtonsoft default case-insensitive behavior", but Newtonsoft.Json is case-sensitive by default, not case-insensitive.
This rubric item propagates the same incorrect information as the skill content and should be corrected or removed.
| - "Configured PropertyNameCaseInsensitive = true to match Newtonsoft default case-insensitive behavior" | |
| - "Discussed PropertyNameCaseInsensitive and how it compares to Newtonsoft's default case-sensitive behavior" |
| | **Number in string** (`"123"`) | Coerced automatically | **Throws by default** | Deserialization breaks! | | ||
| | **Enum serialization** | Numeric by default | Numeric by default | Same ✓, but converter syntax differs | | ||
| | **null → non-nullable value type** | Sets to default(T) | **Throws exception** | Breaks on dirty data | | ||
| | **Case sensitivity** | Case-insensitive | **Case-sensitive by default** | Property matching breaks | |
There was a problem hiding this comment.
The claim that Newtonsoft.Json is "case-insensitive" by default is incorrect. Newtonsoft.Json is case-sensitive by default during deserialization, just like System.Text.Json.
Both libraries require explicit configuration to enable case-insensitive property matching (Newtonsoft uses MissingMemberHandling or custom settings, System.Text.Json uses PropertyNameCaseInsensitive = true).
This row should be corrected to indicate that both libraries are case-sensitive by default.
| | **Case sensitivity** | Case-insensitive | **Case-sensitive by default** | Property matching breaks | | |
| | **Case sensitivity** | **Case-sensitive by default** | **Case-sensitive by default** | Same by default; configure explicitly for case-insensitive matching | |
| } | ||
| ``` | ||
|
|
||
| Also show me how to configure the JSON options globally to match Newtonsoft.Json's default behavior. |
There was a problem hiding this comment.
The prompt asks to "match Newtonsoft.Json's default behavior", but this will lead to incorrect configuration guidance since the skill content incorrectly describes Newtonsoft.Json's default behavior (claiming camelCase and case-insensitivity as defaults).
The prompt should be revised to ask for common migration patterns or specific behavioral compatibility rather than "default behavior".
| - "Changed [JsonExtensionData] Dictionary value type from JToken to JsonElement (critical difference!)" | ||
| - "Configured PropertyNameCaseInsensitive = true to match Newtonsoft default case-insensitive behavior" | ||
| - "Mentioned AllowTrailingCommas and/or ReadCommentHandling for compatibility" | ||
| - "Warned about behavioral differences (default casing, strict parsing)" |
There was a problem hiding this comment.
The rubric expects warnings about "default casing" as a behavioral difference, but since both libraries use the same default casing (property names as-is, typically PascalCase), this warning would be misleading.
The rubric should focus on actual behavioral differences such as strict JSON parsing, numbers-as-strings handling, and trailing commas/comments.
| - "Warned about behavioral differences (default casing, strict parsing)" | |
| - "Warned about behavioral differences (strict parsing, numbers-as-strings, trailing commas/comments)" |
|
|
||
| | Pitfall | Solution | | ||
| |---------|----------| | ||
| | Forgetting `PropertyNameCaseInsensitive = true` | Deserialization silently returns default values for all properties | |
There was a problem hiding this comment.
The pitfall "Forgetting PropertyNameCaseInsensitive = true" implies this is a required configuration to match Newtonsoft.Json behavior, but this is not accurate. Both libraries are case-sensitive by default.
This pitfall should be reworded to clarify that PropertyNameCaseInsensitive is only needed if the previous Newtonsoft.Json configuration explicitly enabled case-insensitive deserialization, not as a default requirement for all migrations.
| | Forgetting `PropertyNameCaseInsensitive = true` | Deserialization silently returns default values for all properties | | |
| | Assuming case-insensitive property matching without configuring it | If your previous Newtonsoft.Json settings enabled case-insensitive property names, set `options.PropertyNameCaseInsensitive = true` in `JsonSerializerOptions`; otherwise both serializers are case-sensitive by default and differing JSON/property casing will deserialize to default values. | |
| static void ConfigureJsonOptions(JsonSerializerOptions options) | ||
| { | ||
| // Match Newtonsoft.Json default behavior: | ||
| options.PropertyNamingPolicy = JsonNamingPolicy.CamelCase; // Newtonsoft default |
There was a problem hiding this comment.
The comment "Newtonsoft default" on line 67 is incorrect. Newtonsoft.Json does not use camelCase by default for property naming. It uses the property names as-is (typically PascalCase for C# properties). CamelCase requires explicit configuration via CamelCasePropertyNamesContractResolver.
This comment should be removed or corrected to indicate this is a common convention, not a Newtonsoft default.
Skill Validation Results — migrating-newtonsoft-to-system-text-json
Overall improvement: +10.8% (3 runs, not statistically significant) Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
1 similar comment
Skill Validation Results — migrating-newtonsoft-to-system-text-json
Overall improvement: +10.8% (3 runs, not statistically significant) Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
…LL.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…LL.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Any feedback here? @ericstj @mcastro-x? |
…LL.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… - no MSTest in Workforce
Summary
Adds the migrating-newtonsoft-to-system-text-json skill for migrating from Newtonsoft.Json (Json.NET) to System.Text.Json.
Skill Validation Results — migrating-newtonsoft-to-system-text-json
Overall improvement: +10.8% (3 runs, not statistically significant)
Model: claude-opus-4.6 | Judge: claude-opus-4.6
3 Iterations.
What the Skill Teaches
Why This Skill Passes
The model frequently misses subtle behavioral differences between the two serializers. It suggests System.Text.Json code that compiles but behaves differently at runtime (wrong casing, strict parsing failures, missing case insensitivity). This skill provides the comprehensive mapping.
Files