SONARPY-1727: S5905: Make sure the quick fix removes trailing commas #2020
Conversation
Seppli11
force-pushed
the
SONARPY-1727
branch
from
f2748b1
to
a647ba8
Compare
Seppli11
force-pushed
the
SONARPY-1727
branch
from
a647ba8
to
283adfc
Compare
| if(tuple.elements().size() == 1 && !tuple.commas().isEmpty()) { | ||
| quickfixBuilder.addTextEdit(TextEditUtils.replaceRange(tuple.commas().get(0), tuple.rightParenthesis(), "")); | ||
| } else { | ||
| quickfixBuilder.addTextEdit(TextEditUtils.remove(tuple.rightParenthesis())); | ||
| } |
There was a problem hiding this comment.
I am not sure this would work with assert (a,b,c,) as this only check for single element tuples. Is that on purpose?
There was a problem hiding this comment.
Oh, ups. No it's not intentional. I wasn't aware that trailing commas are allowed in general 😓
There was a problem hiding this comment.
Now that I'm thinking about this, I'm not sure if the quick fix actually makes sense. For an assert like assert (a,b) there are two ways to correct this with different semantics. it could be assert a, b (where b is the error message), or it could be assert a and b. The current quick fix assumes that it is the first one.
However, given something like assert (a,b,c) the only sensible way to correct that is to generate assert a and b and c. But the current quick fix would generate assert a, b, c which isn't syntactically correct.
There was a problem hiding this comment.
@joke1196, what do you think? Would it make more sense to always join the different elements with and?
There was a problem hiding this comment.
So as discussed we should provide the quick fix only for the single element tuple, as in other cases we risk breaking the code.
| @@ -43,12 +43,18 @@ public void initialize(Context context) { | |||
|
|
|||
| var issue = ctx.addIssue(tuple, MESSAGE); | |||
|
|
|||
| if (tuple.leftParenthesis() != null && tuple.rightParenthesis() != null) { | |||
| if (tuple.leftParenthesis() != null && tuple.rightParenthesis() != null && !tuple.elements().isEmpty()) { | |||
There was a problem hiding this comment.
How do we raise on this test assert () # Noncompliant when the tuple.elements is not empty?
There was a problem hiding this comment.
I was thinking that it wouldn't make sense to offer a quick fix for assert () as this would result in just assert . Or is that a sensible quick fix?
There was a problem hiding this comment.
Oh yes, this makes sense. I thought the check was at the level of the issue detection. And did not get why this would raise! Thanks for the clarification!
| var quickfixBuilder = PythonQuickFix.newQuickFix(QUICK_FIX_MESSAGE) | ||
| .addTextEdit(TextEditUtils.remove(tuple.leftParenthesis())); | ||
|
|
||
| Token lastComma = tuple.commas().get(tuple.commas().size() - 1); | ||
| quickfixBuilder.addTextEdit(TextEditUtils.replaceRange(lastComma, tuple.rightParenthesis(), "")); | ||
|
|
||
| issue.addQuickFix(quickfixBuilder.build()); |
There was a problem hiding this comment.
Nitpick: maybe we can group the quickfixes statement together
| var quickfixBuilder = PythonQuickFix.newQuickFix(QUICK_FIX_MESSAGE) | |
| .addTextEdit(TextEditUtils.remove(tuple.leftParenthesis())); | |
| Token lastComma = tuple.commas().get(tuple.commas().size() - 1); | |
| quickfixBuilder.addTextEdit(TextEditUtils.replaceRange(lastComma, tuple.rightParenthesis(), "")); | |
| issue.addQuickFix(quickfixBuilder.build()); | |
| Token lastComma = tuple.commas().get(tuple.commas().size() - 1); | |
| var quickfixBuilder = PythonQuickFix.newQuickFix(QUICK_FIX_MESSAGE) | |
| .addTextEdit(TextEditUtils.remove(tuple.leftParenthesis())); | |
| .addTextEdit(TextEditUtils.replaceRange(lastComma, tuple.rightParenthesis(), "")); | |
| issue.addQuickFix(quickfixBuilder.build()); |
|
No description provided.