Skip to content

Merge | AssemblyCache - #3236

Merged
benrr101 merged 4 commits into
mainfrom
dev/russellben/merge/assembly-cache
Mar 24, 2025
Merged

Merge | AssemblyCache#3236
benrr101 merged 4 commits into
mainfrom
dev/russellben/merge/assembly-cache

Conversation

@benrr101

@benrr101benrr101 commented Mar 20, 2025

Copy link
Copy Markdown
Contributor

Description: Moving one small class from netfx project to common project. I made a couple changes to it along the way:

  • Removed comments that make literally no sense anymore
  • Made the class static (it only has two static methods in it anymore)
  • Couple low-impact stylistic changes

Change In Plans! Let's just get rid of AssemblyCache! The two remaining methods in the class have been rolled into the respective class they were called in. GetSize was rolled into SqlParameter (only place it was used), GetType was rolled into SqlConnection (which now matches netcore implementation). AssemblyCache was deleted.

Testing: Just moving things around. Still no funny business here.

@benrr101benrr101 added the Common Project 🚮 Things that relate to the common project project label Mar 20, 2025
@benrr101
benrr101 requested review from a team and CopilotMarch 20, 2025 22:56

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

FileDescription
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AssemblyCache.netfx.csAdded and updated AssemblyCache to be static in the common project
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/assemblycache.csRemoved 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

Comment on lines +23 to +24
Debug.Assert(t != null, "Type object cant be NULL");

CopilotAIMar 20, 2025

Copy link

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 object cant be NULL");
Debug.Assert(t!=null,"Type object can't be NULL");

Copilot uses AI. Check for mistakes.
@codecov

codecovBot commented Mar 21, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 62.50000% with 3 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 linesPatch %Lines
...etfx/src/Microsoft/Data/SqlClient/SqlConnection.cs62.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 
FlagCoverage Δ
addons92.58% <ø> (ø)
netcore75.04% <ø> (-0.05%)⬇️
netfx71.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.

@paulmedynskipaulmedynski self-assigned this Mar 21, 2025

@mdaiglemdaigle left a comment

Copy link
Copy Markdown
Contributor

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.

@mdaiglemdaigle added this to the 7.0-preview1 milestone Mar 21, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

FileDescription
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.csReplaced AssemblyCache.GetInfoFromType with an inline helper method.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlParameter.csRemoved conditional branch and now uses SerializationHelperSql9.SizeInBytes directly.
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/assemblycache.csEntire 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);

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Common Project 🚮Things that relate to the common project project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@benrr101@mdaigle@paulmedynski@edwardneal@samsharma2700