Conversation
There was a problem hiding this comment.
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
765cf97 to
1271ba3
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The UDT Type annotation no longer preserves the trimming contract through property accessors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 6
- Review effort level: Balanced
| var udtTypeInfo = new TdsXmlTypeInfo(); | ||
|
|
||
| // Assert | ||
| Assert.Null(udtTypeInfo.Database); | ||
| Assert.Null(udtTypeInfo.Name); | ||
| Assert.Null(udtTypeInfo.OwningSchema); |
1271ba3 to
b857e34
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The UDT Type property loses its trimming data-flow contract, and the new tests omit required XML documentation.
Review details
Suppressed comments (5)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsUdtTypeInfo.cs:45
- Apply
DynamicallyAccessedMembersto the property, not only its compiler-generated backing field. The getter currently exposes an unannotatedType, so calls such asSerializationHelperSql9.Deserializelose the trimming contract and may emit linker warnings or allow required UDT members to be removed.
[field: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Parser/Tokens/TdsXmlTypeInfoTests.cs:9
- The repository's testing guide requires behavior-focused XML summaries on every test class and every test method (
.github/instructions/testing.instructions.md:176-192). Add summaries for this class and both[Fact]methods.
public class TdsXmlTypeInfoTests
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Parser/Tokens/TdsUdtTypeInfoTests.cs:10
- The repository's testing guide requires behavior-focused XML summaries on every test class and every test method (
.github/instructions/testing.instructions.md:176-192). Add summaries for this class and all three[Fact]methods.
public class TdsUdtTypeInfoTests
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Parser/Tokens/TdsTypeInfoTests.cs:11
- The repository's testing guide requires behavior-focused XML summaries on every test class and every test method (
.github/instructions/testing.instructions.md:176-192). Add summaries for this class and all three[Fact]methods.
public class TdsTypeInfoTests
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Parser/Tokens/TdsXmlTypeInfoTests.cs:15
- This variable holds XML type information, so the UDT-oriented name is misleading in this test.
var udtTypeInfo = new TdsXmlTypeInfo();
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
b857e34 to
4212169
Compare
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The trimming annotation and hidden clone method introduce correctness risks, while the new tests also violate repository documentation requirements.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Parser/Tokens/TdsUdtTypeInfo.cs:47
- The
[field:]target annotates only the generated backing field, so the generated property getter does not expose aTypecarrying the required member guarantees. Call sites that passTypeto the UDT serializers can therefore trigger trimming diagnostics or lose the intended preservation. Apply the annotation to the property itself, preserving the semantics of the former annotated field.
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
#endif
internal Type Type { get; set; }
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Parser/Tokens/TdsXmlTypeInfoTests.cs:9
- This new test class and its
[Fact]methods lack the required behavior-focused XML summaries..github/instructions/testing.instructions.md:181-190requires summaries at both class and test-method level; please document each declaration.
public class TdsXmlTypeInfoTests
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Parser/Tokens/TdsUdtTypeInfoTests.cs:10
- This new test class and its
[Fact]methods lack the required behavior-focused XML summaries..github/instructions/testing.instructions.md:181-190requires summaries at both class and test-method level; please document each declaration.
public class TdsUdtTypeInfoTests
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Parser/Tokens/TdsTypeInfoTests.cs:11
- This new test class and its
[Fact]methods lack the required behavior-focused XML summaries..github/instructions/testing.instructions.md:181-190requires summaries at both class and test-method level; please document each declaration.
public class TdsTypeInfoTests
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Parser/Tokens/TdsXmlTypeInfoTests.cs:15
- This XML metadata test names the subject
udtTypeInfo, which makes the assertions appear to cover the wrong metadata kind. Rename it toxmlTypeInfoso the test's intent is unambiguous.
var udtTypeInfo = new TdsXmlTypeInfo();
- Files reviewed: 15/15 changed files
- Comments generated: 3
- Review effort level: Balanced
| internal new TdsColumnMetadata Clone() => | ||
| new TdsColumnMetadata(this); |
| var clone = original.Clone(); | ||
|
|
||
| // Assert | ||
| Assert.Equal(original.AssemblyQualifiedName, clone.AssemblyQualifiedName); |
| var cloned = original.Clone(); | ||
|
|
||
| // Assert | ||
| Assert.Equal(original.Database, cloned.Database); |
There was a problem hiding this comment.
🔵 Needs a closer look
The new tests violate the repository’s mandatory XML documentation convention, and one XML test uses misleading UDT terminology.
Review details
Suppressed comments (4)
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Parser/Tokens/TdsXmlTypeInfoTests.cs:12
- This new test class and its
[Fact]methods omit the behavior-focused XML summaries required by.github/instructions/testing.instructions.md:181-192. Please document the class and each test method, including why the clone/default-value behavior matters.
public class TdsXmlTypeInfoTests
{
[Fact]
public void Constructor_InitializesPropertiesWithDefaultValues()
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Parser/Tokens/TdsTypeInfoTests.cs:14
- This new test class and its
[Fact]methods omit the behavior-focused XML summaries required by.github/instructions/testing.instructions.md:181-192. Please document the class and each test method, including why each clone scenario matters.
public class TdsTypeInfoTests
{
[Fact]
public void Clone_CopiesAllUnclonedProperties()
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Parser/Tokens/TdsXmlTypeInfoTests.cs:15
- This variable contains
TdsXmlTypeInfo, so the UDT-oriented name is misleading in an XML metadata test. Rename it toxmlTypeInfoto keep the test terminology aligned with the type under test.
var udtTypeInfo = new TdsXmlTypeInfo();
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Parser/Tokens/TdsUdtTypeInfoTests.cs:13
- This new test class and its
[Fact]methods omit the behavior-focused XML summaries required by.github/instructions/testing.instructions.md:181-192. Please document the class and each test method, including why the clone/default-value behavior matters.
public class TdsUdtTypeInfoTests
{
[Fact]
public void Constructor_InitializesPropertiesWithDefaultValues()
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Description
Applies the normalized TDS metadata model to user-defined type and XML schema metadata.
SqlMetaDataUdtandSqlMetaDataXmlSchemaCollectiontoTdsUdtTypeInfoandTdsXmlTypeInfo.CopyFrompattern with conventional clone methods and copy constructors across UDT, XML, column, and general type metadata.This is layer 3 of 3 in the TDS parser organization stack. It targets
dev/russellben/tokens/tdstypeinfo, so review should focus on UDT/XML metadata and clone semantics.Issues
No linked issue; this is an internal naming and maintainability refactor.
Testing
Adds focused unit tests for
TdsTypeInfo,TdsUdtTypeInfo, andTdsXmlTypeInfoclone behavior. CI will run the complete dependent branch across the supported target frameworks.Guidelines