Update SonarAnalyzer.CSharp #1494
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduced abstractions for CancellationTokenSource.CancelAsync() and Stream.DisposeAsync() to avoid #ifdef. Supressed pipeStream.WriteAsync because it deadlocks the test. I assume because PipeStream doesn't override WriteAsync. https://rules.sonarsource.com/csharp/RSPEC-6966/ temp
Removed the Connect() from Multifactor_PublicKeyWithEmptyPassPhrase because the Exception is already thrown in the factory. https://rules.sonarsource.com/csharp/RSPEC-3431/
…uld be static This one is pretty redundant with CA1822 (which is also disabled in the tests). It caught a few more cases in the library itself, most of which can't be changed because they are public API. https://rules.sonarsource.com/csharp/RSPEC-2325/
not sure if this one is worth having. The only cases it found are imho legitimate or not worth fixing, so I supressed them. https://rules.sonarsource.com/csharp/RSPEC-127/
…ethods Suppressed for public APIs, changed ExtendedReplyInfo to interface. https://rules.sonarsource.com/csharp/RSPEC-1694/
mus65
requested review from
drieseng,
WojciechNagorski and
Rob-Hague
as code owners
|
IMHO S3993 (attribute usage) and S3431 (ExpectedException) have value, the others do not. But you can wait for other opinions - I don't like the "every analyzer" approach that we take |
Rob-Hague
reviewed
Sep 15, 2024
this is already covered by Test_PrivateKey_SSH2_Encrypted_ShouldThrowSshPassPhraseNullOrEmptyExceptionWhenPassphraseIsNull
|
@mus65 We should globally disable S2325 (and revert the changes that you applied to silence this rule). This is what I have in my .editorconfig at work: # S2325: Methods and properties that don't access instance data should be static
# https://rules.sonarsource.com/csharp/RSPEC-2325
#
# This is a duplicate of CA1822, and reports false positives for ASP.NET Core Middleware
dotnet_diagnostic.S2325.severity = noneCA1822 also allows you to configure the API surface that the rule applies to. We do this for example in the .editorconfig of the Renci.SshNet project. Also S2325 does not appear to respect the |
…data should be static" suppress it instead This reverts commit 2020604.
|
@drieseng done |
|
Sorry for the conflicts, can you also revert/suppress S127 then I'll merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There a quite a few new analyzers. For easier review, I created a separate commit for each analyzer, including a link to the sonar source docs and additional notes from me.
Some of these may be worth disabling instead of adding suppressions. I opted for adding suppressions for now.