sonar-dotnet
sonar-dotnet copied to clipboard
SonarSource
Extend Rule S2681 (put multiline blocks in braces) for additional patterns
I picked up the oldest open Issue (#194) and gave it a go. I first cleaned-up the existing code, and afterwards implemented the additional patterns.
@Corniel your branch has conflicts - perhaps that is why it is not yet reviewed?
@Corniel your branch has conflicts - perhaps that is why it is not yet reviewed?
When I created it, that was not the case. ;) They're busy.
@tbutler-qontigo As long as nobody from Sonar has assigned themselves, it will not be picked up anyway. ;)
@tbutler-qontigo As long as nobody from Sonar has assigned themselves, it will not be picked up anyway. ;)
like a tree falling in the forest with nobody around to hear it.... :)
@tbutler-qontigo As long as nobody from Sonar has assigned themselves, it will not be picked up anyway. ;)
like a tree falling in the forest with nobody around to hear it.... :)
We might not hear it, but we will eventually pick it up 😃 As Corniel said, we're quite busy. And summer is the holiday season too.
@pavel-mikula-sonarsource I added the 4 cases as you suggested. As mentioned, the old test case file is a mess imho; I want to drop it, as it adds more confusion than clarity. However, I suppose we should first have a run with the old file in to prove that we did not break old functionality.
@pavel-mikula-sonarsource I added the 4 cases as you suggested. As mentioned, the old test case file is a mess imho; I want to drop it, as it adds more confusion than clarity. However, I suppose we should first have a run with the old file in to prove that we did not break old functionality.
This plan sounds good to me, as long as we port the old test cases or have their equivalents. I think most of the file can be indeed dropped, as the new one has way better explanation names. As one example, there are the test cases with comments that the new one doesn't have (yet).
This can be done as a last step, after we deal with other comments.
@pavel-mikula-sonarsource I added the 4 cases as you suggested. As mentioned, the old test case file is a mess imho; I want to drop it, as it adds more confusion than clarity. However, I suppose we should first have a run with the old file in to prove that we did not break old functionality.
This plan sounds good to me, as long as we port the old test cases or have their equivalents. I think most of the file can be indeed dropped, as the new one has way better explanation names. As one example, there are the test cases with comments that the new one doesn't have (yet).
This can be done as a last step, after we deal with other comments.
I thought I had them all covered. But that's where reviews are for. :)
I have no new comments for the current state. Please address the older comment about EmptyStatement (add test cases)
![azure-pipelines[bot] avatar](https://avatars.githubusercontent.com/in/9426?v=4 "Published by a pub.dev verified publisher"){kind=small}
@pavel-mikula-sonarsource : I changed the code to ignore cases where at least one of the statements is empty. By doing so, the 'old' test case file start reporting errors. I think I covered al those cases (with the empty ones), and it is safe to delete them. So if you agree, I'll remove it.
We've got bunch of failing ITs. They seem like FPs that should not be there, as we have similar cases in UTs and I'm not sure why they raise. Can you take a look and try to add reproducers in UTs? sources\Nancy\src\Nancy\TinyIoc\TinyIoC.cs, line 1607, 1605, 3059, 3057, sources\akka.net\src\core\Akka\Helios.Concurrency.DedicatedThreadPool.cs, line 152, 153 sources\akka.net\src\core\Akka.Cluster\Reachability.cs, lines 109, 111 ...and so on (see CI).
If one reproducer and fix fixes all, than it's fine to add just one or two UTs
This one is funny (one space behind). We can accept it as TP sources\akka.net\src\core\Akka\IO\InetAddressDnsResolver.cs, line 62, 63, 230, 231, 234, 236
The devil showed up again (in the details, as always):
class SecondPartOfOtherScope
{
void LowerElseIf(bool condition)
{
if (condition)
if (condition)
Act.First("if", "if");
else
Act.First("if", "else");
else if (condition) // Compliant
Act.First("else if");
}
void LowerElse(bool condition)
{
if (condition)
if (condition)
Act.First("if", "if");
else
Act.First("if", "else");
else Act.First("else"); // Compliant
}
void If(bool condition)
{
while (condition)
if(condition)
Act.First("while");
if (condition) Act.First("while", "if"); // Compliant
}
}
I'm thinking of an elegant solution.
@pavel-mikula-sonarsource Solved it. I also added two other cases:
class SecondPartIsPartOfIfStructure
{
void Else(bool condition)
{
if (condition)
Act.First();
else Act.Second(); // Compliant
}
void ElseIf(bool condition)
{
if (condition)
Act.First();
else if(condition) Act.Second(); // Compliant
}
}
Inconvenient, but irrelevant for this rule, as nobody will think that these else(if) statements will also be executed.
Something went wrong, there are many broken cases in ITs. 1st case:
public override bool Equals(object obj)
{
if(obj is null) return false; // probably irrelevant, just pasting as is
return obj is MemberPath path && Equals(path); // Noncompliant FP
}
@pavel-mikula-sonarsource Oops. I found the root cause: by determining the proxy start position (I think that's the best way to name it) the indentation size (of 4) should be added.