SONARPY-1075 Quick fixes for S3923 should not break the code#1177
Conversation
nils-werner-sonarsource
force-pushed
the
SONARPY-1075
branch
from
2538591 to
69fe9bb
Compare
nils-werner-sonarsource
force-pushed
the
SONARPY-1075
branch
from
69fe9bb to
c1af4ac
Compare
|
SonarQube Quality Gate
|
nils-werner-sonarsource
requested review from
andreaguarino
and removed request for
guillaume-dequenne
andreaguarino
left a comment
andreaguarino
left a comment
There was a problem hiding this comment.
LGTM, I'm not sure I saw a test unit for the code snippet reported in the ticket?
Anyway I tested it manually on VSCode and the problem is fixed
| } | ||
|
|
||
| @Test | ||
| public void wrapped_conditional_statements(){ |
There was a problem hiding this comment.
nitpick: conditional_expressions instead of conditional_statements
| " if b == 0:\n" + | ||
| " doSomething(); doOneMoreThing()\n"+ | ||
| " else:\n" + | ||
| " doSomething(); doOneMoreThing()\n"; |
There was a problem hiding this comment.
IMO, for quickfixes we should write "noncompliant" and "fixed" code inside a python file. I think it's not super easy to read code inside a Java string and it might be error prone.
However, if you also agree, let's create a ticket and do that in a separate PR.
| String codeFixed = "doSomething() # false branch comment2\n" + | ||
| " # false branch comment3\n"; |
There was a problem hiding this comment.
can you please add a comment saying we only keep comments of the else branch?
| private static List<Token> tokens(Tree tree) { | ||
| List<Token> tokens = new ArrayList<>(); | ||
| tree.children().forEach(child -> { | ||
| if (child.is(Tree.Kind.TOKEN)) { | ||
| tokens.add((Token) child); | ||
| ((Token) child).trivia().stream() | ||
| .map(Trivia::token) | ||
| .forEach(tokens::add); | ||
| } else { | ||
| tokens.addAll(tokens(child)); | ||
| } | ||
| }); | ||
| return tokens; | ||
| } |
There was a problem hiding this comment.
I think we have already TreeUtils.tokens() that should do this
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; | ||
| PythonTextEdit that = (PythonTextEdit) o; | ||
| return startLine == that.startLine && startLineOffset == that.startLineOffset && endLine == that.endLine | ||
| && endLineOffset == that.endLineOffset && Objects.equals(message, that.message); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(message, startLine, startLineOffset, endLine, endLineOffset); |
| assertThat(edit.equals(new PythonTextEdit("",0, 1, 1, 1))).isFalse(); | ||
| assertThat(edit.equals(new PythonTextEdit("",0, 0, 0, 1))).isFalse(); | ||
| assertThat(edit.equals(new PythonTextEdit("",0, 0, 1, 0))).isFalse(); | ||
| assertThat(edit.equals(new PythonTextEdit("a", 0, 0, 1, 1))).isFalse(); |
There was a problem hiding this comment.
I think we can add a test for hashCode as well
| @@ -45,30 +45,31 @@ public void quickfix_one_statement() { | |||
| " doSomething()\n"; | |||
There was a problem hiding this comment.
I think I didn't see a test for the problematic code showed in the ticket?
More specifically about point 3)
The following statement do_something() gets pushed outside the scope of the function.
| // Remove else branch | ||
| elseBranch.ifPresent(branch -> quickFixBuilder.addTextEdit(PythonTextEdit.remove(branch))); | ||
| // Shift all body statements to the left | ||
| // Skip first shift because already done by upper if statement removal |
There was a problem hiding this comment.
do you mean it's done by the removeUntil?
|
SonarQube Quality Gate
|
GitOrigin-RevId: 403a993d82aec502754212c85c436537787a0c25
2 participants
No description provided.