-
Notifications
You must be signed in to change notification settings - Fork 281
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
Replace unused out arguments with discards #2726
Conversation
src/Microsoft.Data.SqlClient/netcore/src/Common/src/Interop/Windows/sspicli/SSPIWrapper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Common/src/Interop/Windows/sspicli/SSPIWrapper.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benrr101 Thank you for cleaning them up. I believe discarding the redundant arguments makes the code clearer and easier to follow by eliminating distractions.😉
@roji @cheenamalhotra I did a sweep of all the typed discards and removed all the ones I could. This likely touched some files that were untouched before this, but it's verified at compilation. There was one locations that couldn't be changed and that was due to it being ambiguous call if the type was omitted. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2726 +/- ##
==========================================
- Coverage 72.81% 71.75% -1.06%
==========================================
Files 311 308 -3
Lines 61694 62301 +607
==========================================
- Hits 44922 44707 -215
- Misses 16772 17594 +822
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Description: C# 7.0 added support for discards, such that if an out variable is not used after the call to a method that sets it, it can be replaced with
_
to discard to value. There are around 90 instances of this in the codebase, and they are very easy to remove. As far as I can tell this makes no semantic difference to the code, but does clean up the code a bit and bring it up to a more modern standard.The only real argument I could see against making this quick cleanup change is that it reduces the understanding of what the code aims to do. I feel this change is fairly neutral, but please feel free to disagree as necessary.
Testing: Projects still build so it should be good 👍