NotificationsYou must be signed in to change notification settings
Fork311
Star927

Cleanup | SqlDiagnosticListener Classes#3232

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 3 commits intomainfromdev/russellben/cleanup/sqldiagnosticlistener

Mar 20, 2025

Merged

Cleanup | SqlDiagnosticListener Classes#3232

benrr101 merged 3 commits intomainfromdev/russellben/cleanup/sqldiagnosticlistener

Mar 20, 2025

Conversation

Copy link

Contributor

benrr101 commentedMar 19, 2025

Description: Based on the reception of my last cleanup PR, i don't know how this one will be received. This PR just tidies up the SqlDiagnosticListener classes after they were merged.

Removing Name - although a name is required to be passed to the DiagnosticListener class on construction, we only ever use the same one, and the class is internal. Thus, it doesn't make a ton of sense to me to expose the name within the project. So, I removed it from the constructor and just pass in the hardcoded name we use. If we end up having multiple names later, we can reinstate this.
The rest of the changes are just breaking up long lines and some light reordering of member variables.

Testing: Once more, no real functional behavior change. Just a bit of tweaks to syntax and a change that doesn't impact anything external (the name thing).

benrr101 added3 commits

March 19, 2025 14:03

Light cleanup of DiagnosticScope classes

c8b3f7e

Very light cleanup of SqlClient* classes

ea591a0

Some more heavy cleanup on SqlDiagnosticListener

cecc330

benrr101 added the Code Health 💊Issues/PRs that are targeted to source code quality improvements. label

Mar 19, 2025

benrr101 requested a review froma team

March 19, 2025 19:14

mdaigle approved these changes

Mar 19, 2025

View reviewed 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.

Are you using any kind of linter for these changes? I see an editorconfig file checked into the repo, but I don't have much experience with c# linting. Is there a good way to leverage that across IDEs so that we can all follow the same style?

Copy link

codecovbot commentedMar 19, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is58.56481% with179 lines in your changes missing coverage. Please review.

Project coverage is 72.70%. Comparing base(adce139) to head(cecc330).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...lient/Diagnostics/SqlDiagnosticListener.netcore.cs	57.62%	100 Missing⚠️
.../Diagnostics/DiagnosticTransactionScope.netcore.cs	73.68%	15 Missing⚠️
...a/SqlClient/Diagnostics/DiagnosticScope.netcore.cs	59.09%	9 Missing⚠️
...stics/SqlClientTransactionRollbackError.netcore.cs	0.00%	9 Missing⚠️
...agnostics/SqlClientConnectionCloseError.netcore.cs	0.00%	8 Missing⚠️
...nostics/SqlClientTransactionCommitError.netcore.cs	0.00%	8 Missing⚠️
...stics/SqlClientTransactionRollbackAfter.netcore.cs	0.00%	8 Missing⚠️
...tics/SqlClientTransactionRollbackBefore.netcore.cs	0.00%	8 Missing⚠️
...nostics/SqlClientTransactionCommitAfter.netcore.cs	0.00%	7 Missing⚠️
...ostics/SqlClientTransactionCommitBefore.netcore.cs	0.00%	7 Missing⚠️

Additional details and impacted files

@@            Coverage Diff             @@##             main    #3232      +/-   ##==========================================- Coverage   72.76%   72.70%   -0.06%==========================================  Files         303      303                Lines       59527    59712     +185     ==========================================+ Hits        43317    43416      +99- Misses      16210    16296      +86

Flag	Coverage Δ
addons	`92.58% <ø> (ø)`
netcore	`75.08% <58.56%> (-0.09%)`	⬇️
netfx	`71.41% <ø> (-0.02%)`	⬇️

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.

paulmedynski approved these changes

Mar 20, 2025

View reviewed changes

Copy link

ContributorAuthor

benrr101 commentedMar 20, 2025

@mdaigle Sort of... The tooling I use raises a lot more suggestions/hints than plain VS, but it's not raising line length suggestions. Partly because the entire codebase would light up brighter than a christmas tree, partly because I don't know if it's even a supported feature, partly because it may already be turned off in .editorconfig, and partly because I don't really like line length being an enforced thing - I try to follow no lines >120 characters, but sometimes it's only over by a few characters and splitting it would make it less readable. In that situation I don't really want to be forced to split it, I don't want to have it be permanently lit up, and I don't want to have to put (possibly tooling-specific) warning suppressions in the code. It's all very subjective :)