Development of rules GCI300, GCI301, GCI302, GCI303, GCI304, GCI305, GCI306, GCI307, GCI308, GCI309, GCI310, GCI311, GCI312 #66
Conversation
| @@ -40,7 +40,21 @@ public class PythonRuleRepository implements RulesDefinition, PythonCustomRuleRe | |||
| AvoidFullSQLRequest.class, | |||
There was a problem hiding this comment.
Some global feedbacks :
- regarding rule number ... we must follow next number i.e. GCI95 until GCI202
- before implementing rules,
- we must have specifications ready in "creedengo-rules-specifications" repository because of use of this component as dependency
- we must check if each rule should be implemented or not (external argumentation, research paper, ...)
- this PR is too big ... we must use small PR to be more efficient and to deliver more rules
- I think ofr each rule, we should associate the implementation to rule idea created in previous hackathons in creedengo-challenge and creedengo-common repositories (issues tab)
- do you check if there is an existing rule but not yet implemented (in "RULES.md" file in "creedengo-rules-specicifatcions" repository) ?
- the "definition of done" process isn't followed ... please check it : https://github.com/green-code-initiative/creedengo-common/blob/main/doc/starter-pack.md#check-definition-of-done-for-new-rule-implementation
| } | ||
|
|
||
| private void report(StringElement stringElement, SubscriptionContext ctx) { | ||
| private void repport(StringElement stringElement, SubscriptionContext ctx) { |
There was a problem hiding this comment.
i don't understand this modification ... I think "report" is the good name ... because the best practice for a name method is to use a verb and not an noun. a best method name should be "raiseIssue"
| @@ -0,0 +1,101 @@ | |||
| /* | |||
There was a problem hiding this comment.
this kind of Utils class should be in a different package ... the "checks" package is done for implementation rule classes.
I think, we can use "org.greencodeinitiative.creedengo.python" for example
| @@ -24,7 +24,6 @@ public class AvoidSQLRequestInLoopTest { | |||
| @Test | |||
| public void test() { | |||
| PythonCheckVerifier.verify("src/test/resources/checks/avoidSQLRequestInLoop.py", new AvoidSQLRequestInLoop()); | |||
| PythonCheckVerifier.verify("src/test/resources/checks/avoidSQLRequestInLoopCheck.py", new AvoidSQLRequestInLoop()); | |||
There was a problem hiding this comment.
why do you delete this use case ?
maybe rename the test file ... but this is a real use case
| @@ -1,24 +1,70 @@ | |||
| from datetime import date | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
why do you change a lot this test file ? I think your test file is a new use case and the old one is another real use case (even if is not the best practice of Python development)
| @@ -25,7 +25,7 @@ def testWithForLoop(self): | |||
| cursor = db.cursor(dictionary=True) | |||
|
|
|||
| for i in range(0, self.maxrows): | |||
| cursor.execute("SELECT id, name FROM users WHERE id = %(id)s", {'id': i+1}) # Noncompliant {{Avoid performing SQL queries within a loop}} | |||
| cursor.execute("SELECT id, name FROM users WHERE id = %(id)s", {'id': i+1}) #Noncompliant | |||
There was a problem hiding this comment.
why do you delete the error message ? It is mandatory for me ...
Co-authored-by: DataLabGroupe-CreditAgricole GITHUB.DATALABGROUPE@CREDIT-AGRICOLE-SA.FR