Movatterモバイル変換

Copy link

Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR moves the AssemblyCache class from the netfx project to the common project and makes a couple of low-impact adjustments.

Moved the AssemblyCache class to the common project and made it static.
Removed the outdated AssemblyCache implementation from the netfx project.

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AssemblyCache.netfx.cs	Added and updated AssemblyCache to be static in the common project
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/assemblycache.cs	Removed the old AssemblyCache file from the netfx project

Files not reviewed (1)

src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj: Language not supported

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AssemblyCache.netfx.cs Outdated

Comment on lines 23 to 24

		Debug.Assert(t != null, "Type object cant be NULL");

Copy link

Preview

CopilotAIMar 20, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider updating the assertion message to include an apostrophe (e.g. "Type object can't be null") for clarity.

Suggested change

	Debug.Assert(t!=null,"Type objectcant be NULL");
	Debug.Assert(t!=null,"Type objectcan't be NULL");

Copilot uses AI. Check for mistakes.

Copy link

codecovbot commentedMar 21, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is62.50000% with3 lines in your changes missing coverage. Please review.

Project coverage is 72.65%. Comparing base(33083f3) to head(4b66929).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...etfx/src/Microsoft/Data/SqlClient/SqlConnection.cs	62.50%	3 Missing⚠️

Additional details and impacted files

@@            Coverage Diff             @@##             main    #3236      +/-   ##==========================================- Coverage   72.72%   72.65%   -0.07%==========================================  Files         303      302       -1       Lines       59712    59714       +2     ==========================================- Hits        43426    43386      -40- Misses      16286    16328      +42

Flag	Coverage Δ
addons	`92.58% <ø> (ø)`
netcore	`75.04% <ø> (-0.05%)`	⬇️
netfx	`71.38% <62.50%> (-0.06%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 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.

edwardneal reviewed

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AssemblyCache.netfx.cs OutdatedShow resolvedHide resolved

paulmedynski approved these changes

paulmedynski self-assigned this

mdaigle approved these changes

Copy link

Contributor

mdaigle left a comment

Choose a reason for hiding this comment

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

Looks good as is for a pure move. But I also agree with@edwardneal 's method cleanup suggestion.

mdaigle added this to the7.0-preview1 milestone

Forget it, let's just get rid of AssemblyCache!

4b66929

benrr101 requested review fromCopilot,edwardneal,mdaigle andpaulmedynski

March 21, 2025 23:18

CopilotAI reviewed

Copy link

Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the AssemblyCache class and its usages, merging its remaining functionality directly into SqlConnection and SqlParameter.

Removed dependency on AssemblyCache in SqlConnection by inlining the GetInfoFromType logic.
Updated SqlParameter to call SerializationHelperSql9.SizeInBytes instead of using the AssemblyCache branch.
Deleted the AssemblyCache source file.

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File	Description
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs	Replaced AssemblyCache.GetInfoFromType with an inline helper method.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlParameter.cs	Removed conditional branch and now uses SerializationHelperSql9.SizeInBytes directly.
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/assemblycache.cs	Entire file removed as part of deprecation.

Files not reviewed (1)

src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj: Language not supported

Comments suppressed due to low confidence (2)

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs:2728

[nitpick] Consider renaming GetInfoFromType to GetSqlUdtInfoFromType to more clearly indicate its purpose.

SqlUdtInfo attr = GetInfoFromType(o.GetType());

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlParameter.cs:1616

Verify that SerializationHelperSql9.SizeInBytes handles all cases previously managed by AssemblyCache.GetLength, particularly on the NETFX target.

coercedSize = SerializationHelperSql9.SizeInBytes(val);

mdaigle approved these changes

samsharma2700 approved these changes

edwardneal approved these changes

paulmedynski approved these changes