NotificationsYou must be signed in to change notification settings
Fork311
Star927

Merge | Diagnostic Files#3213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

benrr101 merged 6 commits intomainfromdev/russellben/merge/sqldiagnosticlistener

Mar 19, 2025

Merged

Merge | Diagnostic Files#3213

benrr101 merged 6 commits intomainfromdev/russellben/merge/sqldiagnosticlistener

Mar 19, 2025

Conversation

Copy link

Contributor

benrr101 commentedMar 12, 2025

Description: More work as part of the merge project. This one is pretty straightforward - the SqlDiagnostic* classes only belonged to the netcore project, so this PR:

Moves the files over to the common project in a Diagnostics folder
Split the files into one class per file
Moved classes that weren't in the SqlClient.Diagnostics namespace into that namespace
Moved the extension methods for DiagnosticsListener class into the DiagnosticListener class
- I can revert this if there's significant backlash to the change, but considering the code is all internal, to me, there's no real reason to keep these as extension methods.
Also created stubs for SqlCommand and SqlConnection in common project to make it a bit easier to work on the Diagnostic files.

Testing: No functional changes to the code, CI should be able to validate.

benrr101 added6 commits

March 11, 2025 17:13

Move diagnostic files into common project in diagnostics folder

24cda5d

Add stubs for sqlcommand and sqlconnection (for sqldiagnostic work)

d7f97a8

Splitting SqlClientDiagnostic.netcore.cs into one file per class

fb9e78d

Put extension methods back in the listener class itself.

5c7ef3e

Decompose the extensions file into separate files for diagnostic scopes

6bcb79b

Move remaining files to diagnostics namespace

03b2459

benrr101 added the Common Project 🚮Things that relate to the common project project label

Mar 12, 2025

benrr101 requested a review froma team

March 12, 2025 22:17

Copy link

codecovbot commentedMar 13, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is35.74244% with489 lines in your changes missing coverage. Please review.

Project coverage is 72.59%. Comparing base(1e59b88) to head(03b2459).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
...lient/Diagnostics/SqlDiagnosticListener.netcore.cs	57.44%	100 Missing⚠️
...stics/SqlClientTransactionRollbackError.netcore.cs	0.00%	36 Missing⚠️
...agnostics/SqlClientConnectionCloseError.netcore.cs	0.00%	33 Missing⚠️
...nostics/SqlClientTransactionCommitError.netcore.cs	0.00%	33 Missing⚠️
...stics/SqlClientTransactionRollbackAfter.netcore.cs	0.00%	33 Missing⚠️
...tics/SqlClientTransactionRollbackBefore.netcore.cs	0.00%	33 Missing⚠️
...nostics/SqlClientTransactionCommitAfter.netcore.cs	0.00%	30 Missing⚠️
...ostics/SqlClientTransactionCommitBefore.netcore.cs	0.00%	30 Missing⚠️
...iagnostics/SqlClientConnectionOpenError.netcore.cs	36.36%	21 Missing⚠️
...lient/Diagnostics/SqlClientCommandError.netcore.cs	39.39%	20 Missing⚠️
... and8 more

Additional details and impacted files

@@            Coverage Diff             @@##             main    #3213      +/-   ##==========================================+ Coverage   72.58%   72.59%   +0.01%==========================================  Files         289      304      +15       Lines       59503    59367     -136     ==========================================- Hits        43188    43097      -91+ Misses      16315    16270      -45

Flag	Coverage Δ
addons	`92.58% <ø> (ø)`
netcore	`75.12% <35.74%> (+<0.01%)`	⬆️
netfx	`71.22% <ø> (+0.01%)`	⬆️

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.

cheenamalhotra reviewed

Mar 17, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.stub.cs

Copy link

Member

Choose a reason for hiding this comment

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

Is this needed in this PR for merging Diagnostics or can be done later?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

This is only added to aid the migration process - while I'm working within the common project, it resolves missing methods/classes so I can work without red squiggles everywhere. This will be removed when the sqlconnection class is merged.

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlClientDiagnostic.cs

Copy link

Member

cheenamalhotraMar 17, 2025

Choose a reason for hiding this comment

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

I don't see a need to split this class into subclasses since all of them have a single purpose and will not likely see any major works in foreseeable future.
This class could stay in single file targeted for netcore project.

Copy link

ContributorAuthor

benrr101Mar 18, 2025

Choose a reason for hiding this comment

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

No, I disagree, it should be properly split into separate files. It messes with developer expectations to have multiple classes within a single file. C# standards suggest each class should get its own file. There are a lot of bad examples in this codebase where multiple classes have been jammed into the same file, and it makes it much more difficult to know where each class is supposed to be.
If it truly is only used within the confines of another class, maybe we consider making it a nested class.