Skip to content
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

Add static analyzers #126

Merged
merged 13 commits into from
Oct 7, 2018
Merged

Add static analyzers #126

merged 13 commits into from
Oct 7, 2018

Conversation

martincostello
Copy link
Member

This PR adds Roslyn code analyzers (a bit like FxCop of old) that run at build time. It also fixes various issues identified by them including:

  1. Redundant methods caused by changes to IStatsDPublisher over time.
  2. Changes methods that don't use instance data to be static.
  3. Makes the internal DisposableTimer class sealed.
  4. Fixes an incorrectly setup exception for parameter validation.
  5. Uses Array.Empty<T>() to save an allocation in target frameworks that support it.
  6. Specifies ConfigureAwait(false) when awaiting tasks.

Add source analyzers that could find issues with the code at build time through static analysis.
Disable warnings that do not apply to tests or benchmarks.
Remove redundant methods that are no longer used since the interface was changed to remove a number of methods.
Make methods and class static where instance members are not used.
Fix warnings in the integration tests related to string comparisons and case sensitivity.
Fix unused instance warnings by using Assert.Throws() that takes a Func<T> instead of an Action.
Fix warnings about unused members/parameters.
Fix warning for the way IDisposable is implemented by making the internal timer type sealed.
Fix exception being thrown with the wrong type and parameter name.
Use Array.Empty<T>() in target frameworks that support it.
Specify ConfigureAwait(false) when awaiting the tasks in the async timer extensions.
@martincostello martincostello added this to the v4.0.0 milestone Oct 7, 2018
@martincostello martincostello requested a review from a team as a code owner October 7, 2018 12:15
@martincostello
Copy link
Member Author

This PR will probably need rebasing after #117, #118 and #125.

}

[Fact]
public static void InvalidSocketProtocolThrowsInConstructor()
{
Should.Throw<ArgumentOutOfRangeException>(
() => new SocketTransport(LocalStatsEndpoint(), (SocketProtocol)42));
SocketProtocol socketProtocol = (SocketProtocol)42;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use var here? and name it invalidSocketProtocol ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var is fine, but I prefer to have the variable and argument names match for simple tests scenarios like this. Happy to change it if we really want the name like that though.

Copy link
Contributor

@AnthonySteele AnthonySteele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming comments are minor, the rest looks good.

@martincostello martincostello merged commit 188b405 into justeattakeaway:master Oct 7, 2018
@martincostello martincostello deleted the Add-Analyzers branch October 7, 2018 17:27
AnthonySteele pushed a commit to AnthonySteele/NuKeeper that referenced this pull request Oct 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants