Skip to content

Commit

Permalink
Update rules metadata (#1497)
Browse files Browse the repository at this point in the history
  • Loading branch information
guillaume-dequenne-sonarsource authored Jun 21, 2023
1 parent 83b1d7a commit c223d40
Show file tree
Hide file tree
Showing 27 changed files with 712 additions and 382 deletions.
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<h2>Why is this an issue?</h2>
<p>Having to scroll horizontally makes it harder to get a quick overview and understanding of any piece of code.</p>
<p>Scrolling horizontally to see a full line of code lowers the code readability.</p>

Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
<h2>Why is this an issue?</h2>
<p>For better readability, do not put more than one statement on a single line.</p>
<h3>Noncompliant code example</h3>
<p>Putting multiple statements on a single line lowers the code readability and makes debugging the code more complex.</p>
<pre>
if (True): print("hello")
if (True): print("hello") # Noncompliant
</pre>
<h3>Compliant solution</h3>
<p>Write one statement per line to improve readability.</p>
<pre>
if (True):
print("hello")
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
<h2>Why is this an issue?</h2>
<p>Sharing some naming conventions is a key point to make it possible for a team to efficiently collaborate. This rule allows to check that all method
names match a provided regular expression.</p>
<h3>Noncompliant code example</h3>
<p>With default provided regular expression: <code>^[a-z_][a-z0-9_]*$</code></p>
<p>Shared naming conventions allow teams to collaborate efficiently.</p>
<p>This rule raises an issue when a method name does not match a provided regular expression.</p>
<p>For example, with the default provided regular expression <code>^[a-z_][a-z0-9_]*$</code>, the method:</p>
<pre>
class MyClass:
def MyMethod(a,b):
def MyMethod(a,b): # Noncompliant
...
</pre>
<h3>Compliant solution</h3>
<p>should be renamed to</p>
<pre>
class MyClass:
def my_method(a,b):
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
<h2>Why is this an issue?</h2>
<p>Shared coding conventions allow teams to collaborate effectively. This rule allows to check that all class names match a provided regular
expression.</p>
<p>The default regular expression is based on PEP-8 standard. It allows "CapWords" convention and "snake_case" in lowercase. The "snake_case"
convention is accepted by PEP-8 when the class is primarily used as a callable (ex: decorator, context manager, etc…​). However the "CapWords"
convention is recommended in every case.</p>
<h3>Noncompliant code example</h3>
<p>With default provided regular expression <code>^_?([A-Z_][a-zA-Z0-9]*|[a-z_][a-z0-9_]*)$</code>:</p>
<p>Shared naming conventions allow teams to collaborate efficiently.</p>
<p>This rule raises an issue when a class name does not match a provided regular expression.</p>
<p>The default regular expression allows the "CapWords" convention and the "snake_case" in lowercase. The style guide PEP-8 recommends using the
"CapWords" convention in every case but also accepts the "snake_case" convention when the class is primarily used as a callable (ex: decorator,
context manager, etc…​).</p>
<p>For example, with the default provided regular expression <code>^_?([A-Z_][a-zA-Z0-9]*|[a-z_][a-z0-9_]*)$</code>, the classes:</p>
<pre>
class myClass: # Noncompliant
...
Expand All @@ -16,7 +15,7 @@ <h3>Noncompliant code example</h3>
def __exit__(self, type, value, traceback):
pass
</pre>
<h3>Compliant solution</h3>
<p>should be renamed to</p>
<pre>
class MyClass:
...
Expand All @@ -27,4 +26,10 @@ <h3>Compliant solution</h3>
def __exit__(self, type, value, traceback):
pass
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://peps.python.org/pep-0008/#class-names">PEP 8 – Style Guide for Python Code</a> </li>
<li> <a href="https://en.wikipedia.org/wiki/Snake_case">Wikipedia: Snake case</a> </li>
</ul>

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<h2>Why is this an issue?</h2>
<p>A source file that grows too much tends to aggregate too many responsibilities and inevitably becomes harder to understand and therefore to
maintain. Above a specific threshold, it is strongly advised to refactor it into smaller pieces of code which focus on well defined tasks. Those
smaller files will not only be easier to understand but also probably easier to test.</p>
<p>A source file that grows too much tends to aggregate too many responsibilities and inevitably becomes harder to understand and, therefore, to
maintain.</p>
<p>Above a specific threshold, refactor the file into smaller files whose code focuses on well-defined tasks. Those smaller files will be easier to
understand and easier to test.</p>

Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
<h2>Why is this an issue?</h2>
<p>Most of the time a block of code is empty when a piece of code is really missing. So such empty block must be either filled or removed.</p>
<h3>Noncompliant code example</h3>
<p>An empty code block is confusing. It will require some effort from maintainers to determine if it is intentional or indicates the implementation is
incomplete.</p>
<pre>
# Noncompliant: is the block empty on purpose, or is code missing?
for i in range(3):
pass
</pre>
<h3>Exceptions</h3>
<p>When a block contains a comment, this block is not considered to be empty.</p>
<p>Removing or filling the empty code blocks takes away ambiguity and generally results in a more straightforward and less surprising code. ===
Exceptions</p>
<p>The rule ignores code blocks that contain comments.</p>

Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ <h2>Why is this an issue?</h2>
<p><code>FIXME</code> tags are commonly used to mark places where a bug is suspected, but which the developer wants to deal with later.</p>
<p>Sometimes the developer will not have the time or will simply forget to get back to that tag.</p>
<p>This rule is meant to track those tags and to ensure that they do not go unnoticed.</p>
<h3>Noncompliant code example</h3>
<pre>
def divide(numerator, denominator):
return numerator / denominator # FIXME denominator value might be 0
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://cwe.mitre.org/data/definitions/546">MITRE, CWE-546</a> - Suspicious Comment </li>
<li> <a href="https://cwe.mitre.org/data/definitions/546">MITRE, CWE-546 - Suspicious Comment</a> </li>
</ul>

Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,5 @@
546
]
},
"quickfix": "unknown"
"quickfix": "infeasible"
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@
"ruleSpecification": "RSPEC-2190",
"sqKey": "S2190",
"scope": "All",
"quickfix": "unknown"
"quickfix": "infeasible"
}
Original file line number Diff line number Diff line change
@@ -1,22 +1,27 @@
<h2>Why is this an issue?</h2>
<p>The use of operators pairs ( <code>=+</code> or <code>=-</code>) where the reversed, single operator was meant (<code>+=</code> or <code>-=</code>)
will run fine, but not produce the expected results.</p>
<p>This rule raises an issue when <code>=+</code> or <code>=-</code> is used without any spacing between the two operators and when there is at least
one whitespace character after.</p>
<h3>Noncompliant code example</h3>
<p>Using operator pairs (<code>=+</code> or <code>=-</code>) that look like reversed single operators (<code>+=</code> or <code>-=</code>) is
confusing. They compile and run but do not produce the same result as their mirrored counterpart.</p>
<pre>
target = -5
num = 3

target =- num # Noncompliant; target = -3. Is that really what's meant?
target =+ num # Noncompliant; target = 3
target =- num # Noncompliant: target = -3. Is that really what's meant?
target =+ num # Noncompliant: target = 3
</pre>
<h3>Compliant solution</h3>
<p>This rule raises an issue when <code>=+</code> or <code>=-</code> are used without any space between the operators and when there is at least one
whitespace after.</p>
<p>Replace the operators with a single one if that is the intention</p>
<pre>
target = -5
num = 3

target = -num # Compliant; intent to assign inverse value of num is clear
target += num
target -= num # target = -8
</pre>
<p>Or fix the spacing to avoid confusion</p>
<pre>
target = -5
num = 3

target = -num # target = -3
</pre>

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"title": "\"\u003d+\" should not be used instead of \"+\u003d\"",
"title": "Non-existent operators like \"\u003d+\" should not be used",
"type": "BUG",
"status": "ready",
"remediation": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,25 +1,16 @@
<h2>Why is this an issue?</h2>
<p>Calling the <code>not</code> or <code>~</code> prefix operator twice might be redundant: the second invocation undoes the first. Such mistakes are
typically caused by accidentally double-tapping the key in question without noticing. Either this is a bug, if the operator was actually meant to be
called once, or misleading if done on purpose. Calling <code>not</code> twice is commonly done instead of using the dedicated "bool()" builtin
function. However, the latter one increases the code readability and should be used.</p>
<h3>Noncompliant code example</h3>
<p>The repetition of a prefix operator (<code>not</code> or <code>~</code>) is usually a typo. The second operator invalidates the first one:</p>
<pre>
a = 0
b = False

c = not not a # Noncompliant
d = ~~b # Noncompliant
a = False
b = ~~a # Noncompliant: equivalent to "a"
</pre>
<h3>Compliant solution</h3>
<p>While calling <code>not</code> twice is equivalent to calling the <code>bool()</code> built-in function, the latter increases the code readability,
so it should be preferred.</p>
<pre>
a = 0
b = False

c = bool(a)
d = ~b
b = not not a # Noncompliant: use bool()
</pre>
<h3>Exceptions</h3>
<p>If the <code>~</code> function has been overloaded in a customised class and has been called twice, no warning is raised as it is assumed to be an
expected usage.</p>
<p>The rule does not raise an issue if the <code>~</code> function is overloaded in a customized class, as it is assumed to be the expected usage.</p>

Original file line number Diff line number Diff line change
@@ -1,16 +1,12 @@
<h2>Why is this an issue?</h2>
<p>Just because you <em>can</em> do something, doesn’t mean you should, and that’s the case with nested conditional expressions. Nesting conditional
expressions results in the kind of code that may seem clear as day when you write it, but six months later will leave maintainers (or worse - future
you) scratching their heads and cursing.</p>
<p>Instead, err on the side of clarity, and use another line to express the nested operation as a separate statement.</p>
<h3>Noncompliant code example</h3>
<p>Nested conditionals are hard to read and can make the order of operations complex to understand.</p>
<pre>
class Job:
@property
def readable_status(self):
return "Running" if job.is_running else "Failed" if job.errors else "Succeeded" # Noncompliant
</pre>
<h3>Compliant solution</h3>
<p>Instead, use another line to express the nested operation in a separate statement.</p>
<pre>
class Job:
@property
Expand All @@ -21,4 +17,7 @@ <h3>Compliant solution</h3>
</pre>
<h3>Exceptions</h3>
<p>No issue is raised on conditional expressions in comprehensions.</p>
<pre>
job_statuses = ["Running" if job.is_running else "Failed" if job.errors else "Succeeded" for job in jobs] # Compliant by exception
</pre>

Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
<h2>Why is this an issue?</h2>
<p>Cognitive Complexity is a measure of how hard the control flow of a function is to understand. Functions with high Cognitive Complexity will be
difficult to maintain.</p>
<p><a href="https://www.sonarsource.com/docs/CognitiveComplexity.pdf">Cognitive Complexity</a> is a measure of how hard the control flow of a function
is to understand. Functions with high Cognitive Complexity will be difficult to maintain.</p>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://www.sonarsource.com/docs/CognitiveComplexity.pdf">Cognitive Complexity</a> </li>
</ul>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
<h2>Why is this an issue?</h2>
<p>Having the same implementation in every branch of an <code>if</code> chain is an error. Either a copy-paste error was made and something different
should be executed, or there shouldn’t be an <code>if</code> chain at all.</p>
<h3>Noncompliant code example</h3>
<p>Having all branches of an <code>if</code> chain with the same implementation indicates a problem.</p>
<p>In the following code:</p>
<pre>
if b == 0: # Noncompliant
doOneMoreThing()
Expand All @@ -12,8 +11,9 @@ <h3>Noncompliant code example</h3>

b = 4 if a &gt; 12 else 4 # Noncompliant
</pre>
<p>Either there is a copy-paste error that needs fixing or the unnecessary <code>if</code> chain needs removing.</p>
<h3>Exceptions</h3>
<p>This rule does not apply to <code>if</code> chains without <code>else</code>-s.</p>
<p>This rule does not apply to <code>if</code> chains without <code>else</code>.</p>
<pre>
if b == 0: # no issue, this could have been done on purpose to make the code more readable
doOneMoreThing()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,23 +1,16 @@
<h2>Why is this an issue?</h2>
<p>The length of a collection is always greater than or equal to zero. So testing that a length is greater than or equal to zero doesn’t make sense,
since the result is always <code>true</code>. Similarly testing that it is less than zero will always return <code>false</code>. Perhaps the intent
was to check the non-emptiness of the collection instead.</p>
<h3>Noncompliant code example</h3>
<p>The length of a collection is always greater than or equal to zero. Testing it doesn’t make sense, since the result is always
<code>true</code>.</p>
<pre>
mylist = []
if len(myList) &gt;= 0: # Noncompliant
pass

if len(myList) &lt; 0: # Noncompliant
if len(myList) &gt;= 0: # Noncompliant: always true
pass
</pre>
<h3>Compliant solution</h3>
<p>Similarly testing that it is less than zero will always return <code>false</code>.</p>
<pre>
mylist = []
if len(myList) &gt;= 42:
pass

if len(myList) == 0:
if len(myList) &lt; 0: # Noncompliant: always false
pass
----
</pre>
<p>Fix the code to properly check for emptiness if it was the intent, or remove the redundant code to keep the current behavior.</p>

Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
<h2>Why is this an issue?</h2>
<p>It is highly suspicious when a value is saved in a collection for a given key or index and then unconditionally overwritten. Such replacements are
likely errors.</p>
<p>This rule raises an issue when the <a href="https://docs.python.org/3/reference/datamodel.html#object.__setitem__"><code>__setitem__</code></a>
method of the same object is called multiple times with the same index, slice or key without any other action done between the calls.</p>
<h3>Noncompliant code example</h3>
<p>Storing a value inside a collection at a given key or index and then unconditionally overwriting it without reading the initial value is a case of
"dead store".</p>
<pre>
def swap(mylist, index1, index2):
tmp = mylist[index2]
Expand All @@ -18,4 +15,5 @@ <h3>Noncompliant code example</h3>
mymap['a']['b'] = 42
mymap['a']['b'] = 42 # Noncompliant
</pre>
<p>At best it is redundant and will confuse the reader, most often it is an error and not what the developer intended to do.</p>

Original file line number Diff line number Diff line change
@@ -1,33 +1,32 @@
<h2>Why is this an issue?</h2>
<p>When two functions or methods have the same implementation, either it was a mistake - something else was intended - or the duplication was
intentional, but may be confusing to maintainers. In the latter case, one implementation should invoke the other. Numerical and string literals are
not taken into account.</p>
<h3>Noncompliant code example</h3>
<pre>
<p>Two functions having the same implementation are suspicious. It might be that something else was intended. Or the duplication is intentional, which
becomes a maintenance burden.</p>
<pre data-diff-id="1" data-diff-type="noncompliant">
class MyClass:
code = "bounteous"
code = "secret"

def calculate_code(self):
self.do_the_thing()
return self.__class__.code

def get_name(self): # Noncompliant
def get_name(self): # Noncompliant: duplicates calculate_code
self.do_the_thing()
return self.__class__.code

def do_the_thing(self):
pass # on purpose
</pre>
<h3>Compliant solution</h3>
<pre>
<p>If the identical logic is intentional, the code should be refactored to avoid duplication. For example, by having both functions call the same
function or by having one implementation invoke the other.</p>
<pre data-diff-id="1" data-diff-type="compliant">
class MyClass:
code = "bounteous"
code = "secret"

def calculate_code(self):
self.do_the_thing()
return self.__class__.code

def get_name(self):
def get_name(self): # Intent is clear
return self.calculate_code()

def do_the_thing(self):
Expand Down
Loading

0 comments on commit c223d40

Please sign in to comment.