Fix analyzer errors #1221
Fix analyzer errors #1221
Conversation
There was a problem hiding this comment.
I've looked through all the changes from /src and most of /test . It's a lot, so I'd rather you just merge what's there and continue with the next PR.
I don't want to open a conversation about the rules themselves, it's good that they exist and are the same for the entire repo. Forcing formatting is ok for me.
|
I have some comments if you would like to wait for my review. My main concerns are around the number of suppressions: I would rather see such analyzers disabled if we aren't going to comply with them, than to have them enabled and clutter the code with suppressions. |
There was a problem hiding this comment.
To be honest I did not get to the end. I left some comments - a lot of them apply multiple times but I tried not to duplicate any.
My feeling on this PR is that there are a few good changes (langword, concrete types, some new xmldoc), but the noise-to-value ratio is very high.
Analyzers are meant to help with development, but at a certain rate of suppressions and #ifdefs there is an increased cognitive load just to read the code, let alone develop it. We should be careful not to increase the barrier to development.
One other consideration: in a touch-all change like this one, a large number of PRs will develop merge conflicts, increasing the burden to merge them.
| /// <exception cref="SocketException">An error is encountered when resolving <paramref name="hostNameOrAddress"/>.</exception> | ||
| public static IPAddress[] GetHostAddresses(string hostNameOrAddress) | ||
| { | ||
| // TODO Eliminate sync variant, and implement timeout | ||
| /* TODO Eliminate sync variant, and implement timeout */ |
There was a problem hiding this comment.
Don't understand this change (would prefer // as is used everywhere else)
| @@ -13,7 +13,9 @@ public abstract class AuthenticationMethod : IAuthenticationMethod | |||
| /// <value> | |||
| /// The name of the authentication method. | |||
| /// </value> | |||
| #pragma warning disable CA2119 // Seal methods that satisfy private interfaces | |||
There was a problem hiding this comment.
Would rather have the analyzer disabled than a suppression
| @@ -341,7 +341,9 @@ public void Disconnect() | |||
| /// intervals. | |||
| /// </remarks> | |||
| /// <exception cref="ObjectDisposedException">The method was called after the client was disposed.</exception> | |||
| #pragma warning disable S1133 // Deprecated code should be removed | |||
There was a problem hiding this comment.
Would rather have the analyzer disabled than a suppression
| @@ -158,7 +159,9 @@ public uint RemoteChannelNumber | |||
| { | |||
| if (!_remoteChannelNumber.HasValue) | |||
| { | |||
| #pragma warning disable S2372 // Exceptions should not be thrown from property getters | |||
There was a problem hiding this comment.
Would rather have the analyzer disabled than a suppression
| #else | ||
| string.Join(",", allowedAuthenticationMethods))) | ||
| #endif // NET || NETSTANDARD2_1_OR_GREATER | ||
| ; |
There was a problem hiding this comment.
This change does not seem worth the extra clutter. I would rather have this analyzer disabled.
| /// </summary> | ||
| /// <param name="key">The key.</param> | ||
| public HMACMD5(byte[] key) | ||
| #pragma warning disable CA5351 // Do Not Use Broken Cryptographic Algorithms |
There was a problem hiding this comment.
Would rather have the analyzer disabled than a suppression
| /// </summary> | ||
| #pragma warning disable SA1401 // Fields should be private |
There was a problem hiding this comment.
Would rather have the analyzer disabled than a suppression
| @@ -400,7 +438,9 @@ public IAsyncResult BeginExpect(AsyncCallback callback, object state, params Exp | |||
| /// <returns> | |||
| /// An <see cref="IAsyncResult" /> that references the asynchronous operation. | |||
| /// </returns> | |||
| #pragma warning disable CA1859 // Use concrete types when possible for improved performance | |||
There was a problem hiding this comment.
Can't this parameter be tuned to only private methods?
| /// <returns> | ||
| /// Text available in the shell that ends with expected text. | ||
| /// </returns> | ||
| #pragma warning disable CA1822 // Mark members as static |
There was a problem hiding this comment.
Similarly, I'm surprised this flags on a public method
|
|
||
| public async Task InitializeAsync() | ||
| { | ||
| _sshServerImage = new ImageFromDockerfileBuilder() | ||
| .WithName("renci-ssh-tests-server-image") | ||
| .WithDockerfileDirectory(CommonDirectoryPath.GetSolutionDirectory(), Path.Combine("test", "Renci.SshNet.IntegrationTests")) | ||
| .WithDockerfile("Dockerfile") | ||
| .WithDeleteIfExists(true) | ||
|
|
||
| .WithDeleteIfExists(deleteIfExists: true) |
There was a problem hiding this comment.
I am all for named parameters to provide context, but in cases like this and in ConfigureAwait(false) (which is practically a canonical form) it is just noise. I would prefer this analyzer to be disabled
This PR fixes a hugo amount of diagnostics reported by the best analyzers.
Along the way I also fixed a few issues, and I discovered other issues that I'll be fixing later.
Let me know if you want me to split up this PR. I'm ok with that.
This PR enables these analyzers by default, but I'm pretty sure this is not what you want.
It adds quite some cycles to the build.
Lets discuss this.