Commit d5860f1c authored by Grant Paton-Simpson's avatar Grant Paton-Simpson

Add getters/setters advisor; export AST XML when testing

parent fb37215f
Pipeline #558 failed with stages
# https://git.nzoss.org.nz/pyGrant/superhelp
version number: 0.1.15
version number: 0.1.16
author: Grant Paton-Simpson
## Overview
......
......@@ -2,7 +2,7 @@ from setuptools import setup, find_packages # @UnresolvedImport
from codecs import open
from os import path
__version__ = '0.1.15'
__version__ = '0.1.16'
here = path.abspath(path.dirname(__file__))
......
......@@ -9,6 +9,188 @@ 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 getters_setters(block_dets, *, repeated_message=False):
"""
Look for getters and setters and suggest @property if appropriate.
"""
class_els = block_dets.element.xpath(CLASS_XPATH)
if not class_els:
return None
class_getter_setter_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')
if method_name.startswith(('set_', 'get_')):
class_getter_setter_methods[class_name].append(method_name)
if not class_getter_setter_methods:
return None
brief_msg = layout("""\
### Alternative to getters and setters
""")
for class_name, method_names in sorted(class_getter_setter_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 look like
either getters or setters: {nice_list}.
""")
else:
method_type = (
'getter' if method_name.startswith('get_') else 'setter')
brief_msg += layout(f"""\
Class `{class_name}` has a `{method_names[0]}` method that
looks like a {method_type}.
""")
if not repeated_message:
brief_msg += layout("""\
Python doesn't need getters and setters. Instead, you can use
properties. These are easily added using decorators e.g.
`@property`.
""")
main_msg = brief_msg
if not repeated_message:
tm = '\N{TRADE MARK SIGN}'
main_msg += (
layout(f"""\
A good discussion of getters, setters, and properties can be
found at <https://www.python-course.eu/python3_properties.php>.
Getters and setters are usually added in other languages such as
Java because direct attribute access doesn't give the ability to
calculate results or otherwise run a process when a value is
accessed / written.
And it is common for lots of getters and setters to be added,
whether or not they are actually needed - Just In Case{tm}. The
fear is that if you point other code to an attribute, and you
later need to process the attribute or derive it before it is
served up or stored, then you'll need to make a breaking change
to your code. All the client code referencing the attribute will
have to be rewritten to replace direct access with a reference
to the appropriate getter or setter. Understandably then the
inclination is to point to a getter or setter in the first case
even if it doesn't actually do anything different (for now at
least) from direct access. Using these getters and setters is
wasteful, and bloats code unnecessarily, but it avoids the worse
evil of regularly broken interfaces. The benefit is that you can
change implementation later if you need to and nothing will
break. But in Python there is a much better way :-).
Let's compare the getter / setter approach and the property
approach.
#### Using getters / setters
""")
+
layout("""
class Person:
def __init__(self, name):
self.__name = name
def get_name(self):
return self.__name
def set_name(self, name):
if name is not None:
self.__name = name
""", is_code=True)
+
layout("""\
#### Using properties
""")
+
layout("""
class Person:
def __init__(self, name):
self.name = name
@property ## the getter
def name(self):
return self.__name
@name.setter ## the setter
def name(self, name):
if name is not None:
self.__name = name
""", is_code=True)
+
layout("""\
We must define the getter earlier in the script than the setter
because the setter references the getter name. If it is not
already defined above it we will get a `NameError` because
Python doesn't yet know the variable_name part of the
`@<variable_name>.setter`.
In the `\_\_init\_\_` method we can either directly set the
"private" variable `self.__name` (note - unenforced in Python)
or set the public variable `self.name` and pass through the
setter. Passing through the setter makes more sense - presumably
there are some smarts in the setter we want applied otherwise we
wouldn't have gone to the trouble of making it ;-).
You will have also noticed that the method name is defined twice
without the second one overwriting the first (the standard
behaviour). The decorators take care of all that. The only thing
to remember is to use exactly the same attribute name in three
places (assuming both getting and setting):
""")
+
layout("""\
@property
def here(): ## <== 1
...
@here.setter ## <== 2
def here(): ## <== 3
...
""", is_code=True)
)
if repeated_message:
extra_msg = ''
else:
extra_msg = (
layout("""\
Python also has a `deleter` decorator which handle deletion
of the attribute e.g.
""")
+
layout("""\
@name.deleter
def name(self):
...
""", is_code=True)
)
message = {
conf.BRIEF: brief_msg,
conf.MAIN: main_msg,
conf.EXTRA: extra_msg,
}
return message
@filt_block_advisor(xpath=CLASS_XPATH, warning=True)
def selfless_methods(block_dets, *, repeated_message=False):
"""
......@@ -41,7 +223,7 @@ def selfless_methods(block_dets, *, repeated_message=False):
### Method doesn't use instance
""")
for class_name, method_names in class_selfless_methods.items():
for class_name, method_names in sorted(class_selfless_methods.items()):
multiple = len(method_names) > 1
if multiple:
nice_list = get_nice_str_list(method_names, quoter='`')
......@@ -50,6 +232,12 @@ def selfless_methods(block_dets, *, repeated_message=False):
Class `{class_name}` has the following methods that don't use
the instance (usually called `self`): {nice_list}.
""")
else:
brief_msg += layout(f"""\
Class `{class_name}` has a `{method_names[0]}` method that
doesn't use the instance (usually called `self`).
""")
if not repeated_message:
brief_msg += layout("""\
......
......@@ -28,12 +28,12 @@ else:
TEST_SNIPPET = """\
class Demo:
def yell(text):
print(text)
def not_used(self, foo):
print(foo)
def used(self, text):
self.yell(text)
def __init__(self, x):
self.__x = x
def get_x(self):
return self.__x
def set_x(self, x):
self.__x == x
"""
DEMO_SNIPPET = """\
......
......@@ -39,7 +39,7 @@ def check_as_expected(test_conf):
:param list test_conf: list of tuples: snippet, dict of expected message
sources and their expected frequencies
"""
conf.DEV_MODE = True ## updates XML so we can check what is happening :-)
conf.RECORD_AST = True ## updates XML so we can check what is happening :-)
for snippet, expected_source_freqs in test_conf:
messages_dets = get_separated_messages_dets(snippet)
actual_source_freqs = get_actual_source_freqs(
......
......@@ -13,6 +13,7 @@ def test_misc():
{
ROOT + 'one_method_classes': 0,
ROOT + 'selfless_methods': 0,
ROOT + 'getters_setters': 0,
}
),
(
......@@ -23,6 +24,7 @@ def test_misc():
{
ROOT + 'one_method_classes': 1,
ROOT + 'selfless_methods': 0,
ROOT + 'getters_setters': 0,
}
),
(
......@@ -34,6 +36,7 @@ def test_misc():
{
ROOT + 'one_method_classes': 1,
ROOT + 'selfless_methods': 1,
ROOT + 'getters_setters': 0,
}
),
(
......@@ -47,6 +50,7 @@ def test_misc():
{
ROOT + 'one_method_classes': 1,
ROOT + 'selfless_methods': 1,
ROOT + 'getters_setters': 0,
}
),
(
......@@ -58,6 +62,7 @@ def test_misc():
{
ROOT + 'one_method_classes': 1,
ROOT + 'selfless_methods': 1,
ROOT + 'getters_setters': 0,
}
),
(
......@@ -73,6 +78,7 @@ def test_misc():
{
ROOT + 'one_method_classes': 0,
ROOT + 'selfless_methods': 1,
ROOT + 'getters_setters': 0,
}
),
(
......@@ -89,6 +95,7 @@ def test_misc():
{
ROOT + 'one_method_classes': 0,
ROOT + 'selfless_methods': 1,
ROOT + 'getters_setters': 0,
}
),
(
......@@ -103,6 +110,7 @@ def test_misc():
{
ROOT + 'one_method_classes': 1,
ROOT + 'selfless_methods': 1,
ROOT + 'getters_setters': 0,
}
),
(
......@@ -116,6 +124,7 @@ def test_misc():
{
ROOT + 'one_method_classes': 0,
ROOT + 'selfless_methods': 1,
ROOT + 'getters_setters': 0,
}
),
(
......@@ -129,6 +138,7 @@ def test_misc():
{
ROOT + 'one_method_classes': 0,
ROOT + 'selfless_methods': 0,
ROOT + 'getters_setters': 0,
}
),
(
......@@ -143,6 +153,7 @@ def test_misc():
{
ROOT + 'one_method_classes': 0,
ROOT + 'selfless_methods': 1,
ROOT + 'getters_setters': 0,
}
),
(
......@@ -161,6 +172,7 @@ def test_misc():
{
ROOT + 'one_method_classes': 0,
ROOT + 'selfless_methods': 1,
ROOT + 'getters_setters': 0,
}
),
(
......@@ -179,6 +191,23 @@ def test_misc():
{
ROOT + 'one_method_classes': 0,
ROOT + 'selfless_methods': 2,
ROOT + 'getters_setters': 0,
}
),
(
dedent("""\
class Demo:
def __init__(self, x):
self.__x = x
def get_x(self):
return self.__x
def set_x(self, x):
self.__x == x
"""),
{
ROOT + 'one_method_classes': 0,
ROOT + 'selfless_methods': 0,
ROOT + 'getters_setters': 1,
}
),
]
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment