mirror of
https://github.com/dotnet/skills.git
synced 2026-09-20 09:49:54 +08:00
Enhance csharp-refactoring skill documentation and tests
- Updated the skill description to clarify usage and restrictions for refactoring requests. - Added new test cases for preserving serialized contracts in CustomerProfile. - Introduced CustomerProfile class to support new test scenarios.
This commit is contained in:
Wendy Breiding (She/Her)
committed by
Abhitej John
Abhitej John
parent
d450573372
commit
d1a55cc5fd
@@ -1,6 +1,6 @@
|
||||
---
|
||||
name: csharp-refactoring
|
||||
description: "Performs safe, behavior-preserving refactoring of C#/.NET code, verified with build, tests, and analyzers. USE FOR: any request to rename, move, extract, split, modernize, or otherwise restructure C# code without changing behavior, including small requests like 'rename X to Y': rename a symbol/type/file across a solution; move a type or static members to another file/namespace/project; extract a method, interface, or base class; pull members up; inline a method or local; split a large class/file; consolidate or de-duplicate copy-pasted code; sync namespaces to folders; or modernize to current C# idioms (file-scoped namespaces, primary constructors, collection expressions, target-typed new, pattern matching). DO NOT USE TO IMPLEMENT: features, bug fixes, new tests, framework or NuGet upgrades (use dotnet-upgrade), or formatting-only passes (use dotnet format)."
|
||||
description: "Performs safe, behavior-preserving refactoring of C#/.NET code, verified with build, tests, and analyzers. USE FOR: requests to rename, move, extract, split, consolidate, de-duplicate, modernize, or otherwise restructure C# code without changing behavior, including 'rename X to Y', generated/partial declarations, public or serialized contracts, multi-targeted code, and mixed requests where a feature, bug fix, package/framework upgrade, nullability change, or other behavior/contract change is presented as a refactor and must be separated or declined. DO NOT USE FOR: ordinary feature or bug-fix requests that are not framed as refactoring; package/framework upgrades after they have been reclassified (use dotnet-upgrade); new tests; or formatting-only passes (use dotnet format)."
|
||||
license: MIT
|
||||
---
|
||||
|
||||
@@ -19,17 +19,19 @@ When the request changes results, decline the refactor framing and handle it hon
|
||||
- **Framework / NuGet version bump** → not a refactor. Stop this workflow without editing project or
|
||||
package files, explain the reclassification, and redirect to the `dotnet-upgrade` skills. A successful
|
||||
build does not make an upgrade behavior-preserving.
|
||||
- **New feature** (e.g. add a pricing tier, a flag, an endpoint) → not a refactor. Stop this
|
||||
- **New feature** (e.g. add a capability, a flag, or an endpoint) → not a refactor. Stop this
|
||||
workflow and route it to the appropriate feature workflow; do not implement it here. If a separately
|
||||
authorized feature also has a structural cleanup, keep the two changes distinct in the implementation
|
||||
and final report.
|
||||
- **Bug fix or "simplification" that changes output** (e.g. always charge shipping, bump a discount) →
|
||||
- **Bug fix or "simplification" that changes output** (e.g. change a threshold or calculation) →
|
||||
a behavior **change**. It is a legitimate task — do it as an explicit, tested change and update the
|
||||
tests that lock in the new behavior — but keep it **separate** from any refactor and never label it
|
||||
behavior-preserving. Do not stall or report "nothing to change."
|
||||
- **A rename/move with a behavior tweak smuggled in** ("rename X, and while you're there bump the rate")
|
||||
→ split it: do the rename as a behavior-preserving refactor, and treat the tweak as its own tested
|
||||
change, or flag it and defer.
|
||||
tests that lock in the new behavior — but only after it is authorized as a behavior change. Under an
|
||||
explicitly behavior-preserving request, leave that edit undone, complete only any separable structural
|
||||
operation, and report the deferred change. Never label the behavior change behavior-preserving.
|
||||
- **A rename/move with a behavior tweak smuggled in** ("rename X, and while you're there change the result")
|
||||
→ do the rename/move as the behavior-preserving operation and defer the tweak. Perform the tweak only
|
||||
after the user separately accepts it as a tested behavior change; do not silently turn one
|
||||
"behavior-preserving" task into two edits.
|
||||
|
||||
Only when the request is genuinely structure-only do you proceed as a refactor.
|
||||
|
||||
@@ -47,6 +49,25 @@ declaration, and edit the generator input, never generated (`*.g.cs`) output.
|
||||
For the operation → Roslyn-provider mapping and representative PRs, see
|
||||
[references/operation-catalog.md](references/operation-catalog.md).
|
||||
|
||||
## Consolidate toward the existing source of truth
|
||||
|
||||
When de-duplicating, preserve the ownership direction stated by the code or request. If `B` duplicates
|
||||
an implementation already owned by `A`, keep `A` canonical and make `B` delegate to it; do not invert
|
||||
the dependency merely because either direction compiles. Preserve public compatibility wrappers when
|
||||
the duplicate surface is shipped, and migrate only in-repo callers that are safe to move.
|
||||
|
||||
## Preserve contracts beyond C# call sites
|
||||
|
||||
Compilation proves binding compatibility, not every external contract. Before renaming or moving a
|
||||
type/member, check whether its name or metadata is observed by serialization, reflection, dependency
|
||||
injection, configuration binding, source generators, P/Invoke, or `dynamic`.
|
||||
|
||||
| Boundary | Required decision |
|
||||
|---|---|
|
||||
| Serialized/configuration name | Preserve the external name with the repository's existing mechanism (for example, `JsonPropertyName`) while migrating C# callers; run a focused round-trip or payload test. |
|
||||
| Public nullable annotation | Treat tightening or loosening nullability as a source-contract change, not a behavior-preserving refactor. Leave it unchanged unless the contract change is explicitly authorized and validated. |
|
||||
| Uncovered reflection or runtime lookup | Do not guess that a compile-clean rename is safe. Preserve the observed name or stop and report the unverified runtime boundary. |
|
||||
|
||||
## Verify proportionally
|
||||
|
||||
Confirm behavior is preserved after the edit — scaled to blast radius, not a fixed ceremony:
|
||||
|
||||
@@ -20,6 +20,8 @@ stimuli:
|
||||
dest: src/Billing/Billing.csproj
|
||||
- src: src/Billing/CollisionExamples.cs
|
||||
dest: src/Billing/CollisionExamples.cs
|
||||
- src: src/Billing/CustomerProfile.cs
|
||||
dest: src/Billing/CustomerProfile.cs
|
||||
- src: src/Billing/Coupons.cs
|
||||
dest: src/Billing/Coupons.cs
|
||||
- src: src/Billing/Coupons.g.cs.template
|
||||
@@ -59,7 +61,6 @@ stimuli:
|
||||
config:
|
||||
command: dotnet test Fixture.sln --verbosity normal
|
||||
expected_exit_code: 0
|
||||
stdout_matches: &dual-tfm-tests 'Total tests: (?:1[2-9]|[2-9][0-9]|[1-9][0-9]{2,})[\s\S]*Total tests: (?:1[2-9]|[2-9][0-9]|[1-9][0-9]{2,})'
|
||||
timeout: 5m
|
||||
- type: file-contains
|
||||
config:
|
||||
@@ -69,12 +70,6 @@ stimuli:
|
||||
config:
|
||||
path: src/Billing/PublicAPI.Shipped.txt
|
||||
value: static Billing.AppSettingsHelper.ParseIntSetting
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (public API|compatib|obsolete|shim)
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (dotnet (build|test)|rebuild|re-?run[\s\S]{0,30}tests|tests?[\s\S]{0,30}(pass|green))
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Migrates in-repository callers to ParseIntegerSetting while preserving the shipped ParseIntSetting entry point as an obsolete forwarding compatibility shim
|
||||
@@ -101,14 +96,7 @@ stimuli:
|
||||
config:
|
||||
command: dotnet test Fixture.sln --verbosity normal
|
||||
expected_exit_code: 0
|
||||
stdout_matches: *dual-tfm-tests
|
||||
timeout: 5m
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (generated|generator|template)
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (dotnet (build|test)|rebuild|re-?run[\s\S]{0,30}tests|tests?[\s\S]{0,30}(pass|green))
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Finds the generated declaration and the hand-authored partial caller as binding-related parts of one rename
|
||||
@@ -130,8 +118,7 @@ stimuli:
|
||||
text = '\n'.join(path.read_text(encoding='utf-8', errors='ignore') for path in paths);
|
||||
match = re.search(r'class\s+ConfigReader\b[^{]*\{(?P<body>.*?)\n\}', text, re.S);
|
||||
body = match.group('body') if match else '';
|
||||
sys.exit(0 if 'AppSettingsHelper.ParseIntSetting' in body
|
||||
and 'AppSettingsHelper.ParseBoolSetting' in body
|
||||
sys.exit(0 if 'ReadInt' in body and 'ReadBool' in body
|
||||
and 'TryParse' not in body else 1)"
|
||||
expected_exit_code: 0
|
||||
timeout: 1m
|
||||
@@ -143,14 +130,7 @@ stimuli:
|
||||
config:
|
||||
command: dotnet test Fixture.sln --verbosity normal
|
||||
expected_exit_code: 0
|
||||
stdout_matches: *dual-tfm-tests
|
||||
timeout: 5m
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (public API|compatib|shipped|forward)
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (dotnet (build|test)|rebuild|re-?run.{0,20}tests|tests? .{0,20}(pass|green))
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Removes the duplicate parsing implementation by making ConfigReader delegate to AppSettingsHelper
|
||||
@@ -168,16 +148,12 @@ stimuli:
|
||||
config:
|
||||
command: dotnet test Fixture.sln --verbosity normal
|
||||
expected_exit_code: 0
|
||||
stdout_matches: *dual-tfm-tests
|
||||
timeout: 5m
|
||||
# The pass-through wrapper is actually gone.
|
||||
- type: file-not-contains
|
||||
config:
|
||||
path: src/Billing/Pricing.cs
|
||||
value: ApplyTaxWrapper
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (dotnet (build|test)|rebuild|re-?run.{0,20}tests|tests? .{0,20}(pass|green))
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Finds the true binding references to the wrapper before removing it, not just textual matches
|
||||
@@ -195,7 +171,6 @@ stimuli:
|
||||
config:
|
||||
command: dotnet test Fixture.sln --verbosity normal
|
||||
expected_exit_code: 0
|
||||
stdout_matches: *dual-tfm-tests
|
||||
timeout: 5m
|
||||
# The two near-identical classes are actually gone (merged into one type).
|
||||
- type: file-not-contains
|
||||
@@ -206,9 +181,6 @@ stimuli:
|
||||
config:
|
||||
path: src/Billing/Pricing.cs
|
||||
value: class SilverPricing
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (dotnet (build|test)|rebuild|re-?run.{0,20}tests|tests? .{0,20}(pass|green))
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Merges the two classes into a single type parameterized by name and rate, preserving each tier's values
|
||||
@@ -238,9 +210,6 @@ stimuli:
|
||||
config:
|
||||
path: tests/Billing.Tests/Billing.Tests.csproj
|
||||
value: Version="2.8.2"
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (not a .{0,25}refactor|isn'?t a .{0,25}refactor|major version|out of scope|separate .{0,20}(change|workflow)|dotnet-upgrade|redirect|hand[- ]?off)
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Recognizes that a framework or package major-version upgrade is not a behavior-preserving refactor
|
||||
@@ -259,9 +228,6 @@ stimuli:
|
||||
config:
|
||||
path: '**/*.cs'
|
||||
value: Platinum
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (new feature|feature addition|not a .{0,25}refactor|isn'?t a .{0,25}refactor|out of scope|separate .{0,20}(change|workflow)|redirect|hand[- ]?off)
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Recognizes that adding a new pricing tier is a new feature, not a behavior-preserving refactor
|
||||
@@ -284,17 +250,10 @@ stimuli:
|
||||
config:
|
||||
path: src/Billing/OrderProcessor.cs
|
||||
value: subtotal >= 100m
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (separate|follow[- ]?up|behavior[- ]?(preserving|chang)|not (a )?refactor)
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (shipping|threshold|\$50)
|
||||
- type: run-command
|
||||
config:
|
||||
command: dotnet test Fixture.sln --verbosity normal
|
||||
expected_exit_code: 0
|
||||
stdout_matches: *dual-tfm-tests
|
||||
timeout: 5m
|
||||
- type: prompt
|
||||
rubric:
|
||||
@@ -302,6 +261,87 @@ stimuli:
|
||||
- Refuses or defers the $50 shipping-threshold change because it deliberately changes invoice behavior and belongs in a separate tested change
|
||||
- Keeps the existing $100 threshold and does not claim the combined request is behavior-preserving
|
||||
|
||||
- name: Preserve a serialized name while renaming a C# property
|
||||
prompt: "Rename `CustomerProfile.LoyaltyLevel` to `Tier` and migrate its C# callers, but preserve the existing JSON property name so serialized payloads do not change. Verify the solution at Fixture.sln."
|
||||
environment: *fixture-environment
|
||||
graders:
|
||||
- type: file-contains
|
||||
config:
|
||||
path: src/Billing/CustomerProfile.cs
|
||||
value: Tier
|
||||
- type: file-contains
|
||||
config:
|
||||
path: src/Billing/CustomerProfile.cs
|
||||
value: JsonPropertyName("LoyaltyLevel")
|
||||
- type: file-not-contains
|
||||
config:
|
||||
path: tests/Billing.Tests/BillingTests.cs
|
||||
value: LoyaltyLevel = "gold"
|
||||
- type: run-command
|
||||
config:
|
||||
command: dotnet test Fixture.sln --verbosity normal
|
||||
expected_exit_code: 0
|
||||
timeout: 5m
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Renames the C# property and migrates callers without treating a compile-clean rename as proof that the JSON contract is unchanged
|
||||
- Preserves the wire name `LoyaltyLevel` through serialization metadata rather than changing existing payloads to `Tier`
|
||||
- Runs the tests that exercise the serialized payload after the refactor
|
||||
|
||||
- name: Keep a shipped nullable contract out of a refactor
|
||||
prompt: "Simplify `AppSettingsHelper.ParseBoolSetting` by changing its shipped `string? raw` parameter to `string raw`. Treat this nullable-annotation cleanup as behavior-preserving and verify Fixture.sln."
|
||||
environment: *fixture-environment
|
||||
graders:
|
||||
- type: file-contains
|
||||
config:
|
||||
path: src/Billing/AppSettingsHelper.cs
|
||||
value: ParseBoolSetting(string? raw, bool fallback)
|
||||
- type: file-contains
|
||||
config:
|
||||
path: src/Billing/PublicAPI.Shipped.txt
|
||||
value: ParseBoolSetting(string? raw, bool fallback)
|
||||
- type: run-command
|
||||
config:
|
||||
command: dotnet test Fixture.sln --verbosity normal
|
||||
expected_exit_code: 0
|
||||
timeout: 5m
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Recognizes that tightening a nullable annotation on a shipped public API changes its source contract and is not a behavior-preserving cleanup
|
||||
- Leaves both the declaration and shipped API record unchanged instead of forcing callers to satisfy a newly non-null contract
|
||||
- Clearly reclassifies the requested edit as a separately authorized contract change
|
||||
|
||||
- name: Rename one binding without touching same-text symbols
|
||||
prompt: "Rename only `OrderProcessor.DoStuff` to `CalculateInvoice` and migrate its callers. Do not rename unrelated members or text that happen to contain `DoStuff`. Verify Fixture.sln."
|
||||
environment: *fixture-environment
|
||||
graders:
|
||||
- type: file-contains
|
||||
config:
|
||||
path: src/Billing/OrderProcessor.cs
|
||||
value: CalculateInvoice
|
||||
- type: file-contains
|
||||
config:
|
||||
path: tests/Billing.Tests/BillingTests.cs
|
||||
value: CalculateInvoice
|
||||
- type: file-contains
|
||||
config:
|
||||
path: src/Billing/CollisionExamples.cs
|
||||
value: internal int DoStuff()
|
||||
- type: file-contains
|
||||
config:
|
||||
path: src/Billing/CollisionExamples.cs
|
||||
value: DoStuff is shown in migration notes.
|
||||
- type: run-command
|
||||
config:
|
||||
command: dotnet test Fixture.sln --verbosity normal
|
||||
expected_exit_code: 0
|
||||
timeout: 5m
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Renames the requested symbol by binding and updates its callers
|
||||
- Leaves the unrelated same-named member and the migration-note string unchanged
|
||||
- Uses the compiler and tests to catch missed references without applying a repository-wide textual replacement
|
||||
|
||||
- name: Stay dormant for an ordinary feature request
|
||||
prompt: Add a platinum loyalty tier at 15% off and make it available to orders.
|
||||
expect_activation: false
|
||||
|
||||
@@ -0,0 +1,6 @@
|
||||
namespace Billing;
|
||||
|
||||
internal sealed class CustomerProfile
|
||||
{
|
||||
public string LoyaltyLevel { get; init; } = "";
|
||||
}
|
||||
@@ -1,4 +1,5 @@
|
||||
using Billing;
|
||||
using System.Text.Json;
|
||||
using Xunit;
|
||||
|
||||
namespace Billing.Tests;
|
||||
@@ -107,7 +108,9 @@ public class BillingTests
|
||||
public void ConfigReader_MatchesAppSettingsHelper()
|
||||
{
|
||||
Assert.Equal(AppSettingsHelper.ParseIntSetting("10", 0), ConfigReader.ReadInt("10", 0));
|
||||
Assert.Equal(AppSettingsHelper.ParseIntSetting("bad", 7), ConfigReader.ReadInt("bad", 7));
|
||||
Assert.Equal(AppSettingsHelper.ParseBoolSetting("true", false), ConfigReader.ReadBool("true", false));
|
||||
Assert.Equal(AppSettingsHelper.ParseBoolSetting("bad", true), ConfigReader.ReadBool("bad", true));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
@@ -124,4 +127,12 @@ public class BillingTests
|
||||
{
|
||||
Assert.Equal(90m, new Coupons().RedeemDefault(100m));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void CustomerProfile_PreservesSerializedContract()
|
||||
{
|
||||
var profile = new CustomerProfile { LoyaltyLevel = "gold" };
|
||||
|
||||
Assert.Equal("""{"LoyaltyLevel":"gold"}""", JsonSerializer.Serialize(profile));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user