Commit 1fabbae0 authored by Grant Paton-Simpson's avatar Grant Paton-Simpson

Add advisor for methods not using self; _comment -> _msg

parent bff3ac3f
Pipeline #556 canceled with stages
"""
Method advisors are effectively function advisors and are covered there.
"""
from collections import defaultdict
from ..advisors import filt_block_advisor
from .. import conf
from ..utils import layout_comment as layout
from ..utils import get_nice_str_list, layout_comment as layout
CLASS_XPATH = ('descendant-or-self::ClassDef')
@filt_block_advisor(xpath=CLASS_XPATH, warning=True)
def selfless_methods(block_dets, *, repeated_message=False):
"""
Look for class methods that don't use self as candidates for @staticmethod
decorator. Note - not worried about detecting sophisticated cases with
packing etc although it doesn't have to be named "self".
"""
class_els = block_dets.element.xpath(CLASS_XPATH)
if not class_els:
return None
class_selfless_methods = defaultdict(list)
for class_el in class_els:
class_name = class_el.get('name')
method_els = class_el.xpath('body/FunctionDef')
for method_el in method_els:
method_name = method_el.get('name')
arg_els = method_el.xpath('args/arguments/args/arg')
if not arg_els:
continue
first_arg_name = arg_els[0].get('arg')
used_first_names = [
name_el for name_el in method_el.xpath('descendant::Name')
if name_el.get('id') == first_arg_name]
if not used_first_names:
class_selfless_methods[class_name].append(method_name)
if not class_selfless_methods:
return None
brief_msg = layout("""\
### Method doesn't use instance
""")
for class_name, method_names in class_selfless_methods.items():
multiple = len(method_names) > 1
if multiple:
nice_list = get_nice_str_list(method_names, quoter='`')
brief_msg += layout(f"""\
Class `{class_name}` has the following methods that don't use
the instance (usually called `self`): {nice_list}.
""")
if not repeated_message:
brief_msg += layout("""\
If a method doesn't use the instance it can be either pulled into a
function outside the class definition or decorated with
`@staticmethod`. The decorator stops the method expecting the
instance object to be supplied as the first argument.
""")
main_msg = brief_msg
if not repeated_message:
main_msg += (
layout("""
For example, instead of:
""")
+
layout("""
def calc_approx_days(self, years): ## <== self not used!
return round(years * 365.25)
""", is_code=True)
+
layout("""
you could write:
""")
+
layout("""
@staticmethod
def calc_approx_days(years):
return round(years * 365.25)
""", is_code=True)
)
if repeated_message:
extra_msg = ''
else:
extra_msg = layout("""\
It is not obligatory to call the first parameter of a bound method
`self` but you should call it that unless you have a good reason to
break convention.
""")
message = {
conf.BRIEF: brief_msg,
conf.MAIN: main_msg,
conf.EXTRA: extra_msg,
}
return message
@filt_block_advisor(xpath=CLASS_XPATH, warning=True)
def one_method_classes(block_dets, *, repeated_message=False):
"""
......@@ -36,7 +131,7 @@ def one_method_classes(block_dets, *, repeated_message=False):
class_plural = 'es' if multi_sole else ''
class_have_has = 'have' if multi_sole else 'has'
func_plural = 's' if multi_sole else ''
brief_comment = layout(f"""\
brief_msg = layout(f"""\
### Possible option of converting class{class_plural} to single function{func_plural}
......@@ -47,13 +142,13 @@ def one_method_classes(block_dets, *, repeated_message=False):
for class_name, method_name in classes_sole_methods:
method2use = (
f"`{method_name}`" if method_name else 'nothing but `__init__`')
brief_comment += layout(f"""\
brief_msg += layout(f"""\
- {class_name}: {method2use}
""")
if repeated_message:
extra_comment = ''
extra_msg = ''
else:
brief_comment += (
brief_msg += (
layout(f"""\
It may be simpler to replace the class{class_plural} with simple
......@@ -95,7 +190,7 @@ def one_method_classes(block_dets, *, repeated_message=False):
""")
)
extra_comment = layout("""\
extra_msg = layout("""\
Python allows procedural, object-oriented, and functional styles of
programming. Event-based programming is also used in GUI contexts,
for example. Programmers coming to Python from languages that only
......@@ -109,7 +204,7 @@ def one_method_classes(block_dets, *, repeated_message=False):
just function outputs. So, as with most things, it depends.
""")
message = {
conf.BRIEF: brief_comment,
conf.EXTRA: extra_comment,
conf.BRIEF: brief_msg,
conf.EXTRA: extra_msg,
}
return message
......@@ -30,14 +30,14 @@ def decorator_overview(block_dets, *, repeated_message=False):
decorator_names.append(name)
dec_name_list = get_nice_str_list(decorator_names, quoter='`')
plural = 's' if len(decorator_names) > 1 else ''
brief_comment = layout(f"""\
brief_msg = layout(f"""\
### Decorator{plural} used
The code uses the decorator{plural}: {dec_name_list}.
""")
main_comment = brief_comment
main_msg = brief_msg
if not repeated_message:
main_comment += (
main_msg += (
layout("""\
Decorators are a common and handy feature of Python. Using them
......@@ -96,7 +96,7 @@ def decorator_overview(block_dets, *, repeated_message=False):
''', is_code=True)
)
message = {
conf.BRIEF: brief_comment,
conf.MAIN: main_comment,
conf.BRIEF: brief_msg,
conf.MAIN: main_msg,
}
return message
......@@ -5,8 +5,8 @@ from ..utils import get_nice_str_list, layout_comment as layout
ASSIGN_DICT_XPATH = 'descendant-or-self::Assign/value/Dict'
def _get_additional_main_comment(first_name):
additional_main_comment = (
def _get_additional_main_msg(first_name):
additional_main_msg = (
layout(f"""
It is common to iterate through the key-value pairs of a dictionary.
......@@ -45,10 +45,10 @@ def _get_additional_main_comment(first_name):
""", is_code=True)
)
return additional_main_comment
return additional_main_msg
def _get_minimal_dict_details(block_dets, dict_els, plural):
brief_comment = ''
brief_msg = ''
for i, dict_el in enumerate(dict_els):
first = (i == 0)
name = get_assign_name(dict_el)
......@@ -60,20 +60,20 @@ def _get_minimal_dict_details(block_dets, dict_els, plural):
### Dictionar{plural} defined
""")
brief_comment += title
brief_comment += layout(f"""\
brief_msg += title
brief_msg += layout(f"""\
`{name}` is a dictionary with {utils.int2nice(len(items))} items
(i.e. {utils.int2nice(len(items))} mappings).
""")
message = {
conf.BRIEF: brief_comment,
conf.BRIEF: brief_msg,
}
return message
def _get_full_dict_details(block_dets, dict_els, plural):
brief_comment = ''
main_comment = ''
brief_msg = ''
main_msg = ''
first_name = None
for i, dict_el in enumerate(dict_els):
first = (i == 0)
......@@ -87,15 +87,15 @@ def _get_full_dict_details(block_dets, dict_els, plural):
### Dictionar{plural} defined
""")
brief_comment += title
main_comment += title
brief_msg += title
main_msg += title
brief_comment += layout("""\
brief_msg += layout("""\
Dictionaries map keys to values.
""")
main_comment += layout("""\
main_msg += layout("""\
Dictionaries, along with lists, are the workhorses of Python
data structures.
......@@ -121,20 +121,20 @@ def _get_full_dict_details(block_dets, dict_els, plural):
`.values()` method e.g. `{name}`.`values()`.
""")
brief_comment += list_desc
main_comment += list_desc
brief_msg += list_desc
main_msg += list_desc
brief_comment += layout("""\
brief_msg += layout("""\
Keys are unique but values can be repeated.
Dictionaries, along with lists, are the workhorses of Python data
structures.
""")
main_comment += _get_additional_main_comment(first_name)
main_msg += _get_additional_main_msg(first_name)
message = {
conf.BRIEF: brief_comment,
conf.MAIN: main_comment,
conf.BRIEF: brief_msg,
conf.MAIN: main_msg,
conf.EXTRA: layout("""\
Python dictionaries (now) keep the order in which items are added.
......@@ -179,8 +179,8 @@ def mixed_key_types(block_dets, *, repeated_message=False):
Warns about dictionaries with mix of string and integer keys.
"""
dict_els = block_dets.element.xpath(ASSIGN_DICT_XPATH)
brief_comment = ''
main_comment = ''
brief_msg = ''
main_msg = ''
has_mixed = False
mixed_names = []
for i, dict_el in enumerate(dict_els):
......@@ -201,8 +201,8 @@ def mixed_key_types(block_dets, *, repeated_message=False):
### Mix of integer and string keys in dictionary
""")
brief_comment += title
main_comment += title
brief_msg += title
main_msg += title
if not has_mixed:
return None
multiple = len(mixed_names) > 1
......@@ -219,11 +219,11 @@ def mixed_key_types(block_dets, *, repeated_message=False):
`{name}`'s keys include both strings and integers which is probably
a bad idea.
""")
brief_comment += mixed_warning
main_comment += mixed_warning
brief_msg += mixed_warning
main_msg += mixed_warning
if not repeated_message:
main_comment += layout("""\
main_msg += layout("""\
For example, if you have both 1 and "1" as keys in a dictionary
(which is allowed because they are not the same key) it is very easy
......@@ -232,7 +232,7 @@ def mixed_key_types(block_dets, *, repeated_message=False):
""")
message = {
conf.BRIEF: brief_comment,
conf.MAIN: main_comment,
conf.BRIEF: brief_msg,
conf.MAIN: main_msg,
}
return message
......@@ -346,15 +346,15 @@ def manual_incrementing(blocks_dets):
break
if not has_incrementing:
return None
brief_comment = layout(f"""\
brief_msg = layout(f"""\
### Possible option of using `enumerate()`
It looks like your code is manually incrementing `{incrementing_var}`.
In Python you can use the `enumerate` function to handle this for you.
""")
main_comment = (
brief_comment
main_msg = (
brief_msg
+
layout("""\
......@@ -404,7 +404,7 @@ def manual_incrementing(blocks_dets):
""")
)
message = {
conf.BRIEF: brief_comment,
conf.MAIN: main_comment,
conf.BRIEF: brief_msg,
conf.MAIN: main_msg,
}
return message
......@@ -38,17 +38,17 @@ def exception_overview(blocks_dets):
exception_blocks = get_exception_blocks(blocks_dets)
if not exception_blocks:
return None
brief_comment = '### Exception handling'
brief_msg = '### Exception handling'
for n, exception_block in enumerate(exception_blocks, 1):
counter = '' if len(exception_blocks) == 1 else f" {int2nice(n)}"
brief_comment += layout(f"""\
brief_msg += layout(f"""\
#### `try`-`except` block{counter}
""")
brief_comment += _get_exception_block_comment(exception_block)
brief_msg += _get_exception_block_comment(exception_block)
message = {
conf.BRIEF: layout(brief_comment),
conf.BRIEF: layout(brief_msg),
}
return message
......@@ -60,7 +60,7 @@ def unspecific_exception(blocks_dets):
exception_blocks = get_exception_blocks(blocks_dets)
if not exception_blocks:
return None
brief_comment = ''
brief_msg = ''
unspecific_block_ns = []
has_title = False
has_unspecific = False
......@@ -72,7 +72,7 @@ def unspecific_exception(blocks_dets):
has_unspecific = True
unspecific_block_ns.append(n)
if not has_title:
brief_comment += layout("""\
brief_msg += layout("""\
#### Un-specific `Exception` only in `try`-`except` block(s)
......@@ -89,7 +89,7 @@ def unspecific_exception(blocks_dets):
for unspecific_block_n in unspecific_block_ns]
blocks_ns = get_nice_str_list(unspecific_nice_block_ns, quoter='')
block_n_specific_text = f"s {blocks_ns} have"
brief_comment += layout(f"""\
brief_msg += layout(f"""\
`try`-`except` block{block_n_specific_text} an un-specific Exception
only.
......@@ -101,9 +101,9 @@ def unspecific_exception(blocks_dets):
For example:
""")
message = {
conf.BRIEF: layout(brief_comment),
conf.BRIEF: layout(brief_msg),
conf.MAIN: (
layout(brief_comment)
layout(brief_msg)
+
layout("""\
......
......@@ -40,21 +40,21 @@ def comprehension_option(block_dets, *, repeated_message=False):
comp_comment = shared.SET_COMPREHENSION_COMMENT
else:
return None
brief_comment = layout(f"""\
brief_msg = layout(f"""\
### Possible option of using a {comp_type}
""")
if not repeated_message:
brief_comment += layout(f"""\
brief_msg += layout(f"""\
Simple for loops can sometimes be replaced with comprehensions. In
this case a simple reading of the code suggests a {comp_type} might
be possible. Of course, only use a comprehension if it makes your
code easier to understand.
""")
message = {
conf.BRIEF: brief_comment,
conf.BRIEF: brief_msg,
conf.MAIN: (
brief_comment
brief_msg
+
shared.GENERAL_COMPREHENSION_COMMENT + '\n\n' + comp_comment
),
......@@ -140,7 +140,7 @@ def for_index_iteration(block_dets, *, repeated_message=False):
break
if not any_incremental_iteration:
return None
brief_comment = layout(f"""\
brief_msg = layout(f"""\
### Possible option of using direct iteration
......@@ -148,7 +148,7 @@ def for_index_iteration(block_dets, *, repeated_message=False):
indexes. In Python you can iterate directly which is much easier.
""")
if not repeated_message:
brief_comment += (
brief_msg += (
layout(f"""\
For example, instead of:
......@@ -182,7 +182,7 @@ def for_index_iteration(block_dets, *, repeated_message=False):
""")
)
message = {
conf.BRIEF: brief_comment,
conf.BRIEF: brief_msg,
}
return message
......@@ -201,7 +201,7 @@ def nested_fors(block_dets, *, repeated_message=False):
break
if not nested_iteration:
return None
brief_comment = layout("""\
brief_msg = layout("""\
### Possible option of simplifying nested iteration
......@@ -209,7 +209,7 @@ def nested_fors(block_dets, *, repeated_message=False):
""")
if not repeated_message:
brief_comment += (
brief_msg += (
layout("""\
For example, you could replace:
......@@ -234,9 +234,9 @@ def nested_fors(block_dets, *, repeated_message=False):
print(f"{person} might like a {pet} in {year}")
""", is_code=True)
)
main_comment = brief_comment
main_msg = brief_msg
if not repeated_message:
main_comment += layout("""\
main_msg += layout("""\
Whether this is a good idea or not depends on your specific code but
using `product` has the advantage of reducing indentation. It also
......@@ -245,7 +245,7 @@ def nested_fors(block_dets, *, repeated_message=False):
of items.
""")
message = {
conf.BRIEF: brief_comment,
conf.MAIN: main_comment,
conf.BRIEF: brief_msg,
conf.MAIN: main_msg,
}
return message
......@@ -197,7 +197,7 @@ def func_overview(block_dets, *, repeated_message=False):
"""
func_els = block_dets.element.xpath(FUNC_DEFN_XPATH)
overall_func_type_lbl = get_overall_func_type_lbl(func_els)
brief_comment = layout(f"""\
brief_msg = layout(f"""\
### {overall_func_type_lbl.title()} Details
""")
for func_el in func_els:
......@@ -207,17 +207,17 @@ def func_overview(block_dets, *, repeated_message=False):
func_el, repeated_message=repeated_message)
exit_comment = _get_exit_comment(
func_el, func_type_lbl, repeated_message=repeated_message)
brief_comment += layout(f"""\
brief_msg += layout(f"""\
The {func_type_lbl} named `{name}` {arg_comment}. {exit_comment}.
""")
main_comment = brief_comment
main_msg = brief_msg
if repeated_message:
extra_comment = ''
extra_msg = ''
else:
if func_type_lbl == conf.METHOD_LBL:
main_comment += layout("""\
main_msg += layout("""\
Methods are functions that sit directly inside a class
definition. Unless they are defined as static methods e.g. using
......@@ -225,7 +225,7 @@ def func_overview(block_dets, *, repeated_message=False):
class as the first parameter - almost always named `self`. But
they are basically functions.
""")
extra_comment = layout(f"""\
extra_msg = layout(f"""\
There is often confusion about the difference between arguments and
parameters. {overall_func_type_lbl.title()}s define parameters but
receive arguments. You can think of parameters as being like car
......@@ -233,8 +233,8 @@ def func_overview(block_dets, *, repeated_message=False):
to a {overall_func_type_lbl} depending on its parameters.
""")
message = {
conf.BRIEF: brief_comment,
conf.EXTRA: extra_comment,
conf.BRIEF: brief_msg,
conf.EXTRA: extra_msg,
}
return message
......@@ -245,7 +245,7 @@ def func_len_check(block_dets, *, repeated_message=False):
"""
func_els = block_dets.element.xpath(FUNC_DEFN_XPATH)
overall_func_type_lbl = get_overall_func_type_lbl(func_els)
brief_comment = ''
brief_msg = ''
has_short_comment = False
for func_el in func_els:
func_type_lbl = get_func_type_lbl(func_el)
......@@ -260,25 +260,25 @@ def func_len_check(block_dets, *, repeated_message=False):
continue
else:
if not has_short_comment:
brief_comment += layout(f"""\
brief_msg += layout(f"""\
### {overall_func_type_lbl.title()} possibly too long
""")
has_short_comment = True
brief_comment += layout(f"""\
brief_msg += layout(f"""\
`{name}` has {utils.int2nice(func_lines_n)} lines of code
(including comments but with empty lines ignored).
""")
if not repeated_message:
brief_comment += (
brief_msg += (
f" Sometimes it is OK for a {func_type_lbl} to be "
"that long but you should consider refactoring the code "
"into smaller units.")
if not brief_comment:
if not brief_msg:
return None
message = {
conf.BRIEF: brief_comment,
conf.BRIEF: brief_msg,
}
return message
......@@ -295,7 +295,7 @@ def func_excess_parameters(block_dets, *, repeated_message=False):
Warn about functions that might have too many parameters.
"""
func_els = block_dets.element.xpath(FUNC_DEFN_XPATH)
brief_comment = ''
brief_msg = ''
has_high = False
overall_func_type_lbl = get_overall_func_type_lbl(func_els)
for func_el in func_els:
......@@ -304,17 +304,17 @@ def func_excess_parameters(block_dets, *, repeated_message=False):
n_args = get_n_args(func_el)
high_args = n_args > conf.MAX_BRIEF_FUNC_ARGS
if high_args:
brief_comment += layout(f"""\
brief_msg += layout(f"""\
### Possibly too many {overall_func_type_lbl} parameters
""")
brief_comment += layout(f"""\
brief_msg += layout(f"""\
`{name}` has {n_args:,} parameters.
""")
if not (has_high or repeated_message):
brief_comment += layout(f"""\
brief_msg += layout(f"""\
Sometimes it is OK for a {func_type_lbl} to have that many
but you should consider refactoring the code or collecting
related parameters into single parameters e.g. instead of
......@@ -325,7 +325,7 @@ def func_excess_parameters(block_dets, *, repeated_message=False):
if not has_high:
return None
message = {
conf.BRIEF: brief_comment,
conf.BRIEF: brief_msg,
}
return message
......@@ -359,7 +359,7 @@ def positional_boolean(block_dets, *, repeated_message=False):
"""
func_els = block_dets.element.xpath(FUNC_DEFN_XPATH)
overall_func_type_lbl = get_overall_func_type_lbl(func_els)
brief_comment = ''
brief_msg = ''
has_positional_comment = False
for func_el in func_els:
func_type_lbl = get_func_type_lbl(func_el)
......@@ -367,13 +367,13 @@ def positional_boolean(block_dets, *, repeated_message=False):
danger_args = get_danger_args(func_el)
if danger_args:
if not has_positional_comment:
brief_comment += layout(f"""\
brief_msg += layout(f"""\
### {overall_func_type_lbl.title()} expects risky positional
arguments
""")
if not repeated_message:
brief_comment += layout(f"""\
brief_msg += layout(f"""\
{func_type_lbl.title}s which expect numbers or booleans
(True/False) without requiring keywords are risky. They are
......@@ -382,12 +382,12 @@ def positional_boolean(block_dets, *, repeated_message=False):
more intelligible than greeting(True). And intelligible code
is safer to alter / maintain over time than mysterious code.
""")
brief_comment += layout(f"""\
brief_msg += layout(f"""\
A partial analysis of `{name}` found the following risky non-
keyword (positional) parameters: {danger_args}.
""")
if not (has_positional_comment) or repeated_message:
brief_comment += (
brief_msg += (
layout("""\
Using an asterisk as a pseudo-parameter forces all
......@@ -410,9 +410,9 @@ def positional_boolean(block_dets, *, repeated_message=False):
if not has_positional_comment:
return None
if repeated_message:
extra_comment = ''
extra_msg = ''
else:
extra_comment = layout(f"""\
extra_msg = layout(f"""\
Putting an asterisk in the parameters has the effect of forcing all
parameters to the right to be keyword parameters because the
asterisk mops up any remaining positional arguments supplied (if
......@@ -421,8 +421,8 @@ def positional_boolean(block_dets, *, repeated_message=False):
only keyword parameters are allowed thereafter.
""")
message = {
conf.BRIEF: brief_comment,
conf.EXTRA: extra_comment,
conf.BRIEF: brief_msg,
conf.EXTRA: extra_msg,