Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 20, 2025

Conversation

benrr101
Copy link
Contributor

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 benrr101 added the Code Health 💊 Issues/PRs that are targeted to source code quality improvements. label Mar 19, 2025
@benrr101 benrr101 requested a review from a team March 19, 2025 19:14
Copy link
Contributor

@mdaigle 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

codecov bot commented Mar 19, 2025

Codecov Report

Attention: Patch coverage is 58.56481% with 179 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.

@benrr101
Copy link
Contributor Author

@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 :)

@benrr101 benrr101 merged commit 33083f3 into main Mar 20, 2025
252 checks passed
@benrr101 benrr101 deleted the dev/russellben/cleanup/sqldiagnosticlistener branch March 20, 2025 20:48
@mdaigle mdaigle added this to the 7.0-preview1 milestone Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Health 💊 Issues/PRs that are targeted to source code quality improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants