Repository navigation
Make SA1117 work with multiline arguments - #3115
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3115 +/- ##
=======================================
Coverage 97.13% 97.14%
=======================================
Files 841 841
Lines 104943 104903 -40
Branches 3416 3402 -14
=======================================
- Hits 101934 101905 -29
+ Misses 2095 2089 -6
+ Partials 914 909 -5 |
|
@sharwell @vweijsters We at Lombiq would sponsor the review and integration of this fix if you're open to it. |
sharwell
changed the title
|
@pantosha I am planning to experiment and see if I can clean up some of this slightly. Do you mind if I just update the pull request with the result by pushing extra commits to your branch? |
|
@sharwell feel free to modify my PR. |
sharwell
left a comment
sharwell
left a comment
There was a problem hiding this comment.
I already updated this from master branch to simplify some of the review, and am in the process of applying the changes noted in comments. I believe it will be ready for merge once I complete this step.
| } | ||
|
|
||
| private static void Analyze(SyntaxNodeAnalysisContext context, SeparatedSyntaxList<ParameterSyntax> parameters) | ||
| private static void Analyze(SyntaxNodeAnalysisContext context, IReadOnlyList<SyntaxNode> arguments) |
There was a problem hiding this comment.
💡 This can be made generic to avoid boxing at the call sites.
| private static void Analyze(SyntaxNodeAnalysisContext context, IReadOnlyList<SyntaxNode> arguments) | |
| private static void Analyze<T>(SyntaxNodeAnalysisContext context, SeparatedSyntaxList<T> arguments) | |
| where T : SyntaxNode |
There was a problem hiding this comment.
➡️ Fixed!
@pantosha This was a case where the original code was unnecessarily complex to apply a performance optimization. Your initial change was a dramatic improvement to readability, and together we also made it just as fast 👍
| { | ||
| // arguments should be on same line | ||
| lineCondition = (param1Line, param2Line) => param1Line == param2Line; | ||
| lineCondition = (param1, param2) => param1.GetLine() == param2.GetEndLine(); |
There was a problem hiding this comment.
📝 This one I'm not so sure about. For now I'm going to relax the condition:
| lineCondition = (param1, param2) => param1.GetLine() == param2.GetEndLine(); | |
| lineCondition = (param1, param2) => param1.GetLine() == param2.GetLine(); |
This change means we still fix #3106, but it doesn't address #3183 which is still in discussion.
There was a problem hiding this comment.
➡️ This one is now changed back to only compare starting lines.
|
@pantosha no action is needed from you at this time; just giving you a heads-up regarding the changes being made |
3 participants
Fix #3106