respect attribute namespace during migration

[SimonCropp]

No description provided.

[claude]

Code Review: respect attribute namespace during migration

Summary: This PR fixes a real and non-obvious bug — the migration analyzers were matching attributes by name only, so a user-defined [MyCompany.Attributes.Ignore] would get incorrectly migrated as if it were [NUnit.Framework.Ignore]. Using the semantic model to verify the containing namespace before converting is the right approach.

---

Design

The chosen architecture is clean:

• A single abstract IsFrameworkNamespace(string? ns) to keep framework-specific knowledge in each concrete analyzer
• A concrete IsFrameworkAttribute(AttributeSyntax node) in the base class handles the semantic lookup and composes the null-fallback logic centrally
• The guard is placed before both the removal check and the conversion check, so the fix is applied consistently to both paths

This is exactly the right level of abstraction. No concerns here.

---

One Minor Observation

IsFrameworkNamespace has a string? parameter, but the base-class caller already guards the null case:

var ns = symbol?.ContainingType?.ContainingNamespace?.ToDisplayString();

if (ns == null)
    return true; // If we can't resolve, assume framework (preserves existing behavior)

return IsFrameworkNamespace(ns); // ns is guaranteed non-null here

All three implementations then re-check ns != null defensively, which is harmless but redundant. The parameter could be string (non-nullable) since the abstract method is never called with null. This is a very minor point and either style is acceptable — nullable is arguably safer as a defensive API surface.

---

Fallback Behavior

Returning true (treat as framework attribute) when the symbol can't be resolved is the right conservative default. It preserves existing behavior for edge cases where semantic information isn't available, avoiding false negatives in the migration tool.

---

Test Coverage

The parameterized tests are well-chosen — they enumerate the most common attribute names that could realistically collide with user-defined types, and verify the full round-trip through the code fixer. Coverage spans all three frameworks. Good work.

---

Verdict: This is a correct, well-structured fix with solid tests. The only observation above is cosmetic and doesn't warrant a change. Approving.

[claude]

Code Review: respect attribute namespace during migration

Summary: This PR fixes a real and non-obvious bug — the migration analyzers were matching attributes by name only, so a user-defined [MyCompany.Attributes.Ignore] would get incorrectly migrated as if it were [NUnit.Framework.Ignore]. Using the semantic model to verify the containing namespace before converting is the right approach.

---

Design

The chosen architecture is clean:

• A single abstract IsFrameworkNamespace(string? ns) keeps framework-specific knowledge in each concrete analyzer
• A concrete IsFrameworkAttribute(AttributeSyntax node) in the base class handles the semantic lookup and composes the null-fallback logic centrally
• The guard is placed before both the removal check and the conversion check, so the fix is applied consistently to both paths

This is exactly the right level of abstraction. No concerns here.

---

One Minor Observation

IsFrameworkNamespace has a string? parameter, but the base-class caller already guards the null case:

var ns = symbol?.ContainingType?.ContainingNamespace?.ToDisplayString();

if (ns == null)
    return true; // If we can't resolve, assume framework (preserves existing behavior)

return IsFrameworkNamespace(ns); // ns is guaranteed non-null here

All three implementations then re-check ns != null defensively, which is harmless but redundant. The parameter could be string (non-nullable) since the abstract method is never called with null. Minor point — either style is acceptable.

---

Fallback Behavior

Returning true (treat as framework attribute) when the symbol can't be resolved is the right conservative default. It preserves existing behavior for edge cases where semantic information isn't available, avoiding false negatives in the migration tool.

---

Test Coverage

The parameterized tests are well-chosen — they enumerate the most common attribute names that could realistically collide with user-defined types, and verify the full round-trip through the code fixer. Coverage spans all three frameworks (NUnit, MSTest, xUnit). Good work.

---

Verdict: Correct, well-structured fix with solid tests. Approved.

[thomhurst]

Thanks!