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.

Sign up for GitHub

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

Jump to bottom

Rule S3717: Track use of 'NotImplementedException' #603

Merged
merged 4 commits into from
Jul 26, 2017
Merged

Rule S3717: Track use of 'NotImplementedException' #603

merged 4 commits into from
Jul 26, 2017

Conversation

Evangelink
Copy link
Contributor

Fix #585

michalb-sonar
michalb-sonar suggested changes Jul 26, 2017
View reviewed changes
Copy link
Contributor

@michalb-sonar michalb-sonar left a comment

Choose a reason for hiding this comment

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

Nice simple code and so many catches!

sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/TrackNotImplementedException.cs Outdated

var typeInfo = c.SemanticModel.GetTypeInfo(throwStatement.Expression);

if (typeInfo.Type == null ||
Copy link
Contributor

Choose a reason for hiding this comment

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

This if with return can be ommited - Type.Is already covers these cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad!

sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/TrackNotImplementedException.cs
@@ -0,0 +1,13 @@
using System;

namespace Tests.Diagnostics
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add more test cases:

  1. Create a type inheriting from NotImplementedException. Should we report on that?
  2. Create an exception, but do not throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add the tests. I don't think we should handle sub-classes of NotImplementedException.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine with that (as long as you add tests).

Should we mention that exception in RSPEC text?

@Evangelink Evangelink merged commit 3305ef2 into master Jul 26, 2017
@Evangelink Evangelink deleted the rule-S3717 branch July 26, 2017 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valhristov valhristov Awaiting requested review from valhristov

@michalb-sonar michalb-sonar Awaiting requested review from michalb-sonar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Evangelink @michalb-sonar