Skip to content

Commit eff26c8

Browse files
guillaume-dequenneguillaume-dequenne
authored
SONARPY-1497 Add a quickfix for rule S6735 (#1597)
1 parent 6191010 commit eff26c8

File tree

3 files changed

+136
-29
lines changed

3 files changed

+136
-29
lines changed

‎python-checks/src/main/java/org/sonar/python/checks/PandasAddMergeParametersCheck.java

Lines changed: 59 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -24,22 +24,25 @@
2424
import java.util.Map;
2525
import java.util.Optional;
2626
import java.util.Set;
27+
import java.util.stream.Collectors;
2728
import org.sonar.check.Rule;
2829
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
2930
import org.sonar.plugins.python.api.SubscriptionContext;
31+
import org.sonar.plugins.python.api.quickfix.PythonQuickFix;
3032
import org.sonar.plugins.python.api.symbols.Symbol;
3133
import org.sonar.plugins.python.api.tree.Argument;
3234
import org.sonar.plugins.python.api.tree.CallExpression;
3335
import org.sonar.plugins.python.api.tree.Tree;
36+
import org.sonar.python.quickfix.TextEditUtils;
3437
import org.sonar.python.tree.TreeUtils;
3538

3639
@Rule(key = "S6735")
3740
public class PandasAddMergeParametersCheck extends PythonSubscriptionCheck {
3841

3942
enum Keywords {
40-
HOW("how", 2, 1, 2),
41-
ON("on", 1, 2, 3),
42-
VALIDATE("validate", 6, 11, 12);
43+
HOW("how", 2, 1, 2, "\"inner\"", "\"left\""),
44+
ON("on", 1, 2, 3, "None", "None"),
45+
VALIDATE("validate", 6, 11, 12, "\"many_to_many\"", "\"many_to_many\"");
4346

4447
public String getKeyword() {
4548
return keyword;
@@ -62,24 +65,52 @@ public int getPositionInPandasMerge() {
6265
final int positionInDataFrameMerge;
6366
final int positionInPandasMerge;
6467

65-
Keywords(String keyword, int positionInJoin, int positionInDataFrameMerge, int positionInPandasMerge) {
68+
final String defaultValueMerge;
69+
final String defaultValueJoin;
70+
71+
String getReplacementText(String fullyQualifiedName) {
72+
if (DATAFRAME_JOIN_FQN.equals(fullyQualifiedName)) {
73+
return String.format("%s=%s", this.keyword, this.defaultValueJoin);
74+
} else {
75+
return String.format("%s=%s", this.keyword, this.defaultValueMerge);
76+
}
77+
}
78+
79+
int getArgumentPosition(String fullyQualifiedName) {
80+
if (DATAFRAME_JOIN_FQN.equals(fullyQualifiedName)) {
81+
return this.getPositionInJoin();
82+
} else if (DATAFRAME_MERGE_FQN.equals(fullyQualifiedName)) {
83+
return this.getPositionInDataFrameMerge();
84+
}
85+
return this.getPositionInPandasMerge();
86+
}
87+
88+
Keywords(String keyword, int positionInJoin, int positionInDataFrameMerge, int positionInPandasMerge, String defaultValueMerge, String defaultValueJoin) {
6689
this.keyword = keyword;
6790
this.positionInJoin = positionInJoin;
6891
this.positionInDataFrameMerge = positionInDataFrameMerge;
6992
this.positionInPandasMerge = positionInPandasMerge;
93+
this.defaultValueMerge = defaultValueMerge;
94+
this.defaultValueJoin = defaultValueJoin;
7095
}
7196
}
7297

98+
private static final String DATAFRAME_JOIN_FQN = "pandas.core.frame.DataFrame.merge";
99+
private static final String DATAFRAME_MERGE_FQN = "pandas.core.frame.DataFrame.join";
100+
private static final String PANDAS_MERGE_FQN = "pandas.core.reshape.merge.merge";
101+
73102
private static final Set<String> METHODS = Set.of(
74-
"pandas.core.frame.DataFrame.merge",
75-
"pandas.core.reshape.merge.merge",
76-
"pandas.core.frame.DataFrame.join");
103+
DATAFRAME_JOIN_FQN,
104+
DATAFRAME_MERGE_FQN,
105+
PANDAS_MERGE_FQN);
77106

78107
private static final Map<Integer, String> MESSAGES = Map.of(
79108
1, "Specify the \"%s\" parameter of this %s.",
80109
2, "Specify the \"%s\" and \"%s\" parameters of this %s.",
81110
3, "Specify the \"%s\", \"%s\" and \"%s\" parameters of this %s.");
82111

112+
private static final String QUICKFIX_MESSAGE = "Add the missing parameters";
113+
83114
@Override
84115
public void initialize(Context context) {
85116
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, PandasAddMergeParametersCheck::verifyMergeCallParameters);
@@ -94,42 +125,44 @@ private static void verifyMergeCallParameters(SubscriptionContext ctx) {
94125
}
95126

96127
private static void missingArguments(String fullyQualifiedName, SubscriptionContext ctx, CallExpression callExpression) {
97-
List<String> parameters = new ArrayList<>();
128+
List<Keywords> missingKeywords = new ArrayList<>();
98129
if (isArgumentMissing(fullyQualifiedName, Keywords.HOW, callExpression.arguments())) {
99-
parameters.add(Keywords.HOW.getKeyword());
130+
missingKeywords.add(Keywords.HOW);
100131
}
101132
if (isArgumentMissing(fullyQualifiedName, Keywords.ON, callExpression.arguments())) {
102-
parameters.add(Keywords.ON.getKeyword());
133+
missingKeywords.add(Keywords.ON);
103134
}
104135
if (isArgumentMissing(fullyQualifiedName, Keywords.VALIDATE, callExpression.arguments())) {
105-
parameters.add(Keywords.VALIDATE.getKeyword());
136+
missingKeywords.add(Keywords.VALIDATE);
106137
}
107-
if (!parameters.isEmpty()) {
108-
ctx.addIssue(callExpression, generateMessage(MESSAGES.get(numberOfMissingArguments(parameters)), parameters, fullyQualifiedName));
138+
if (!missingKeywords.isEmpty()) {
139+
PreciseIssue issue = ctx.addIssue(callExpression, generateMessage(MESSAGES.get(numberOfMissingArguments(missingKeywords)), missingKeywords, fullyQualifiedName));
140+
issue.addQuickFix(PythonQuickFix
141+
.newQuickFix(QUICKFIX_MESSAGE)
142+
.addTextEdit(
143+
TextEditUtils.insertBefore(callExpression.rightPar(), getReplacementText(fullyQualifiedName, missingKeywords)))
144+
.build());
109145
}
110146
}
111147

112148
private static boolean isArgumentMissing(String fullyQualfiedName, Keywords keyword, List<Argument> arguments) {
113149
return Optional.of(keyword)
114-
.map(kw -> TreeUtils.nthArgumentOrKeyword(getArgumentPosition(fullyQualfiedName, kw), kw.getKeyword(), arguments))
150+
.map(kw -> TreeUtils.nthArgumentOrKeyword(kw.getArgumentPosition(fullyQualfiedName), kw.getKeyword(), arguments))
115151
.isEmpty();
116152
}
117153

118-
private static int getArgumentPosition(String fullyQualifiedName, Keywords parameter) {
119-
if ("pandas.core.frame.DataFrame.join".equals(fullyQualifiedName)) {
120-
return parameter.getPositionInJoin();
121-
} else if ("pandas.core.frame.DataFrame.merge".equals(fullyQualifiedName)) {
122-
return parameter.getPositionInDataFrameMerge();
123-
}
124-
return parameter.getPositionInPandasMerge();
154+
private static String generateMessage(String message, List<Keywords> missingKeywords, String functionName) {
155+
List<String> missingKeywordsList = missingKeywords.stream().map(Keywords::getKeyword).collect(Collectors.toList());
156+
missingKeywordsList.add(functionName.substring(functionName.lastIndexOf('.') + 1));
157+
return String.format(message, missingKeywordsList.toArray());
125158
}
126159

127-
private static String generateMessage(String message, List<String> missingKeywords, String functionName) {
128-
missingKeywords.add(functionName.substring(functionName.lastIndexOf('.') + 1));
129-
return String.format(message, missingKeywords.toArray());
160+
private static int numberOfMissingArguments(List<Keywords> missingKeywords) {
161+
return missingKeywords.size();
130162
}
131163

132-
private static int numberOfMissingArguments(List<String> keywords) {
133-
return keywords.size();
164+
private static String getReplacementText(String fullyQualifiedName, List<Keywords> missingKeywords) {
165+
return String.format(", %s", missingKeywords.stream().map(keyword -> keyword.getReplacementText(fullyQualifiedName))
166+
.collect(Collectors.joining(", ")));
134167
}
135168
}

‎python-checks/src/test/java/org/sonar/python/checks/PandasAddMergeParametersCheckTest.java

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,87 @@
2020
package org.sonar.python.checks;
2121

2222
import org.junit.jupiter.api.Test;
23+
import org.sonar.python.checks.quickfix.PythonQuickFixVerifier;
2324
import org.sonar.python.checks.utils.PythonCheckVerifier;
2425

2526
class PandasAddMergeParametersCheckTest {
27+
public static final PandasAddMergeParametersCheck CHECK = new PandasAddMergeParametersCheck();
28+
2629
@Test
2730
void test() {
28-
PythonCheckVerifier.verify("src/test/resources/checks/pandasAddMergeParameters.py", new PandasAddMergeParametersCheck());
31+
PythonCheckVerifier.verify("src/test/resources/checks/pandasAddMergeParameters.py", CHECK);
32+
}
33+
34+
@Test
35+
void quickfix_test_1() {
36+
final String non_compliant = "def non_compliant_merge_1():\n" +
37+
" import pandas as pd\n" +
38+
"\n" +
39+
" age_df = pd.read_csv(\"age_csv.csv\")\n" +
40+
" name_df = pd.read_csv(\"name_csv.csv\")\n" +
41+
"\n" +
42+
" _ = age_df.merge(name_df)";
43+
44+
final String compliant = "def non_compliant_merge_1():\n" +
45+
" import pandas as pd\n" +
46+
"\n" +
47+
" age_df = pd.read_csv(\"age_csv.csv\")\n" +
48+
" name_df = pd.read_csv(\"name_csv.csv\")\n" +
49+
"\n" +
50+
" _ = age_df.merge(name_df, how=\"left\", on=None, validate=\"many_to_many\")";
51+
PythonQuickFixVerifier.verify(CHECK, non_compliant, compliant);
52+
PythonQuickFixVerifier.verifyQuickFixMessages(CHECK, non_compliant, "Add the missing parameters");
2953
}
54+
55+
@Test
56+
void quickfix_test_2() {
57+
final String non_compliant = "def non_compliant_merge_1():\n" +
58+
" import pandas as pd\n" +
59+
"\n" +
60+
" age_df = pd.read_csv(\"age_csv.csv\")\n" +
61+
" name_df = pd.read_csv(\"name_csv.csv\")\n" +
62+
"\n" +
63+
" _ = age_df.merge(name_df, on=\"user_id\")";
64+
65+
final String compliant = "def non_compliant_merge_1():\n" +
66+
" import pandas as pd\n" +
67+
"\n" +
68+
" age_df = pd.read_csv(\"age_csv.csv\")\n" +
69+
" name_df = pd.read_csv(\"name_csv.csv\")\n" +
70+
"\n" +
71+
" _ = age_df.merge(name_df, on=\"user_id\", how=\"left\", validate=\"many_to_many\")";
72+
73+
PythonQuickFixVerifier.verify(CHECK, non_compliant, compliant);
74+
PythonQuickFixVerifier.verifyQuickFixMessages(CHECK, non_compliant, "Add the missing parameters");
75+
}
76+
77+
78+
@Test
79+
void quickfix_test_3() {
80+
final String non_compliant = "def non_compliant_merge_1():\n" +
81+
" import pandas as pd\n" +
82+
"\n" +
83+
" age_df = pd.read_csv(\"age_csv.csv\")\n" +
84+
" name_df = pd.read_csv(\"name_csv.csv\")\n" +
85+
"\n" +
86+
" _ = pd.merge(\n" +
87+
" age_df, \n" +
88+
" name_df, \n" +
89+
" on=\"user_id\")";
90+
91+
final String compliant = "def non_compliant_merge_1():\n" +
92+
" import pandas as pd\n" +
93+
"\n" +
94+
" age_df = pd.read_csv(\"age_csv.csv\")\n" +
95+
" name_df = pd.read_csv(\"name_csv.csv\")\n" +
96+
"\n" +
97+
" _ = pd.merge(\n" +
98+
" age_df, \n" +
99+
" name_df, \n" +
100+
" on=\"user_id\", how=\"inner\", validate=\"many_to_many\")";
101+
102+
PythonQuickFixVerifier.verify(CHECK, non_compliant, compliant);
103+
PythonQuickFixVerifier.verifyQuickFixMessages(CHECK, non_compliant, "Add the missing parameters");
104+
}
105+
30106
}

‎python-checks/src/test/java/org/sonar/python/checks/PandasDataFrameToNumpyCheckTest.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,6 @@ class PandasDataFrameToNumpyCheckTest {
2727

2828
PandasDataFrameToNumpyCheck check = new PandasDataFrameToNumpyCheck();
2929

30-
private static final String QUICK_FIX_MESSAGE = "Replace this with a call to DataFrame.to_numpy()";
31-
3230
@Test
3331
void test() {
3432
PythonCheckVerifier.verify("src/test/resources/checks/pandasDataFrameToNumpy.py", check);

0 commit comments

Comments
 (0)