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

Update behavior of SA1501 when SA1503 is disabled #1189

Merged
merged 6 commits into from
Aug 13, 2015

Conversation

sharwell
Copy link
Member

This commit updates SA1501 to include special handling for the case where
SA1503 is disabled. In this scenario, statements are analyzed instead of
curly braces.

Fixes #1175

This commit updates SA1501 to include special handling for the case where
SA1503 is disabled. In this scenario, statements are analyzed instead of
curly braces.

Fixes DotNetAnalyzers#1175
{
if (i == 0)
Debug.Assert(true);
else
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 Not sure yet what causes this result from the code fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

❓ What happens in this case for testCode?

if (i == 0)
    Debug.Assert(true); else Debug.Assert(false);

Copy link
Member Author

Choose a reason for hiding this comment

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

In the updated implementation, I believe this does not report SA1501 at all.

@sharwell
Copy link
Member Author

There are a couple remaining buggy behaviors in the code fix for this, but it does seem to work decently well overall. I'm a bit short on time so I configured the tests to detect the bugs as the expected behavior and left comments for review. Let me know whether you'd like to see them fixed before merging or if bug reports would be acceptable.

@sharwell sharwell added this to the 1.0.0 Beta 6 milestone Aug 12, 2015
This commit addresses some inconsistencies by only reporting the first
violation of SA1501 on a line. Note that this only applies to cases where
SA1503 is disabled.
}
}";

var batchFixedTestCode = @"using System.Diagnostics;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since batchFixedTestCode now matches fixedTestCode should it be removed?

@Noryoko
Copy link
Contributor

Noryoko commented Aug 12, 2015

❓ Can you add tests for else and else if statements?

@Noryoko
Copy link
Contributor

Noryoko commented Aug 12, 2015

if (i == 0)
    Debug.Assert(true);
else Debug.Assert(false);
if (i == 0) Debug.Assert(true); else if (i == 0) Debug.Assert(false);
if (i == 0)
    Debug.Assert(true);
else if (i == 0) Debug.Assert(false);
if (i == 0) Debug.Assert(true); else if (i == 0) Debug.Assert(false); else Debug.Assert(false);
if (i == 0) if (i == 0) Debug.Assert(false); else Debug.Assert(false); else Debug.Assert(true);
if (i == 0)
    if (i == 0) Debug.Assert(false); else Debug.Assert(false); else Debug.Assert(true);
if (i == 0) if (i == 0) Debug.Assert(false);
    else Debug.Assert(false); else if (i == 0) Debug.Assert(true);

@Noryoko
Copy link
Contributor

Noryoko commented Aug 13, 2015

👍

sharwell added a commit that referenced this pull request Aug 13, 2015
Update behavior of SA1501 when SA1503 is disabled
@sharwell sharwell merged commit 33f8be7 into DotNetAnalyzers:master Aug 13, 2015
@sharwell sharwell deleted the fix-1175 branch August 13, 2015 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants