Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

pep8 naming for Issue #44 #121

Closed
wants to merge 11 commits into from

3 participants

@stephan-hof

Hello,

here is a series of patches which bring checkers for naming conventions.
Its not fully complete, for example checks for class members are completely missing. Right now the following checks are there

  • Class names
  • Function names
  • Method names
  • Function argument names
  • Local variable names

I added some tests which are working under python2.7 and python3.2.

To make the tests possible I had to enhance the 'run_tests' method, because some syntax features of python3 are not available under python2. For example forcing keyword arguments.
My change checks the first three lines of the file to test. If this file contains a comment with 'python3 only' but pep8.py running under python2 the complete file is omitted. I added the same for 'python2 only'.

Feedback is very welcome

@florentx
Owner

Thank you for this wonderful feature.
It might help when designing a new software to make it compliant with the conventions of PEP 8.

However, I don't plan to merge it in the next version. It is a little bit disruptive for existing libraries and it could give a lot of noise on large projects, while developers might have plenty of (bad) reasons to circumvent the naming conventions of PEP 8.
Moreover, it seems that the AST functions used in the patch are not compatible with Python 2.5.

I've written a short statement about the current choice (even if it might change some day):
https://pep8.readthedocs.org/en/latest/intro.html#disclaimer

@stephan-hof

I understand the points you made in the docs. Would it make sense, to enable this feature only if the user requested it. For example via a command line flag, disabled by default. So the users of the tool can choose if they use it or not.

Regarding py2.5, I will take a look on this.

@myint

You may already know this, but pylint does something similar. pylint checks for conformance to a naming convention, which can be specified by the user.

@florentx florentx commented on the diff
((143 lines not shown))
+ elif not self.GOOD_FUNCTION_NAME.match(node.name):
+ self.error_at_node(node, self.text)
+ elif not self.GOOD_FUNCTION_NAME.match(node.name):
+ self.error_at_node(node, self.text)
+
+
+class FunctionArgNamesASTCheck(BaseAstCheck):
+ """
+ The argument names of a function should be lowercase, with words separated
+ by underscores.
+
+ A classmethod should have 'cls' as first argument.
+ A method should have 'self' as first argument.
+ """
+
+ GOOD_ARG_NAME = re.compile('[a-z_][a-z0-9_]{0,30}$').match
@florentx Owner

is the 31 chars limit in the PEP 8 recommendations?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stephan-hof

No pep8 does not enforce the length.

I saw that you merged my pull request on a separate branch, where you did already a lot of changes (including the drop of the length check). I looked quickly over it and all the changes make sense to me. When I'm back from holidays, I will have a closer look at it. I will also use your branch for daily work so it sees some testing.

Is there anything else I can do right now to help you on this issue? Otherwise this pull request can be closed?

@florentx
Owner

Please try flint and flint-naming.

@florentx florentx closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 12, 2012
  1. @stephan-hof
  2. @stephan-hof
  3. @stephan-hof
  4. @stephan-hof
  5. @stephan-hof

    Allow it to ignore tests completly if the python version does not match

    stephan-hof authored
    Having the new AST based checkes, the AST generation will sometimes fail in
    python2. For example if syntax features are used not supported in python2,
    like keywordonly arguments. If these test are executed under python2
    they will definitely fail
  6. @stephan-hof
  7. @stephan-hof
  8. @stephan-hof

    Keep the stack of parents.

    stephan-hof authored
    Makes it possible for some checkers to get outer function defintion
  9. @stephan-hof
  10. @stephan-hof

    Add checker for local variables inside functions

    stephan-hof authored
    Do not signal globals as local variables
Commits on Aug 31, 2012
  1. @stephan-hof
This page is out of date. Refresh to see the latest.
View
298 pep8.py
@@ -95,6 +95,7 @@ def blank_lines(logical_line, blank_lines, indent_level, line_number)
__version__ = '1.3.4a0'
+import ast
import os
import sys
import re
@@ -104,6 +105,7 @@ def blank_lines(logical_line, blank_lines, indent_level, line_number)
import tokenize
from optparse import OptionParser
from fnmatch import fnmatch
+from collections import deque
try:
from configparser import RawConfigParser
from io import TextIOWrapper
@@ -112,6 +114,7 @@ def blank_lines(logical_line, blank_lines, indent_level, line_number)
DEFAULT_EXCLUDE = '.svn,CVS,.bzr,.hg,.git'
DEFAULT_IGNORE = 'E24'
+IS_PY3 = sys.version_info[0] == 3
if sys.platform == 'win32':
DEFAULT_CONFIG = os.path.expanduser(r'~\.pep8')
else:
@@ -158,6 +161,8 @@ def blank_lines(logical_line, blank_lines, indent_level, line_number)
# a comment which is on a line by itself.
COMMENT_WITH_NL = tokenize.generate_tokens(['#\n'].pop).send(None)[1] == '#\n'
+IS_PY3_TEST = re.compile("^#\s*python3\s*only")
+IS_PY2_TEST = re.compile("^#\s*python2\s*only")
##############################################################################
# Plugins (check functions) for physical lines
@@ -1020,6 +1025,267 @@ def python_3000_backticks(logical_line):
##############################################################################
+# Checkers on AST
+##############################################################################
+class BaseAstCheck(object):
+ """Base class all ASTChecker should derive"""
+
+ def __init__(self, checker):
+ self.checker = checker
+ self.report_error = checker.report_error
+
+ def default_visit(self, node, parents):
+ """Function which is called if not appropiate vist_ method is found"""
+ pass
+
+ def error_at_node(self, node, text):
+ self.report_error(node.lineno, node.col_offset, text, self)
+
+ def get_parent_function(self, parents):
+ for parent in reversed(parents):
+ if isinstance(parent, ast.FunctionDef):
+ return parent
+ if isinstance(parent, ast.ClassDef):
+ return None
+ return None
+
+
+class VisitorsRunner(object):
+ def __init__(self, visitors):
+ self.visitors = visitors
+ self.parents = deque()
+
+ def run(self, node):
+ self.visit_node(node)
+ self.parents.append(node)
+ for child in ast.iter_child_nodes(node):
+ self.run(child)
+ self.parents.pop()
+
+ def visit_node(self, node):
+ if isinstance(node, ast.ClassDef):
+ self.tag_class_functions(node)
+
+ if isinstance(node, ast.FunctionDef):
+ self.find_global_defs(node)
+
+ method = 'visit_' + node.__class__.__name__
+ # Dont break pep8 in a tool to check pep8
+ method = method.lower()
+ for visitor in self.visitors:
+ meth = getattr(visitor, method, visitor.default_visit)
+ meth(node, self.parents)
+
+ def tag_class_functions(self, cls_node):
+ """Tag functions if they are methods, classmethods, staticmethods"""
+
+ # tries to find all 'old style decorators' like
+ # m = staticmethod(m)
+ late_decoration = {}
+ for node in ast.iter_child_nodes(cls_node):
+ if not isinstance(node, ast.Assign):
+ continue
+
+ if not isinstance(node.value, ast.Call):
+ continue
+
+ if not isinstance(node.value.func, ast.Name):
+ continue
+
+ func_name = node.value.func.id
+ if func_name in ('classmethod', 'staticmethod'):
+ if len(node.value.args) == 1:
+ late_decoration[node.value.args[0].id] = func_name
+
+ # iterate over all functions and tag them
+ for node in ast.iter_child_nodes(cls_node):
+ if not isinstance(node, ast.FunctionDef):
+ continue
+
+ if node.name in late_decoration:
+ node.function_type = late_decoration[node.name]
+
+ elif node.decorator_list:
+ decos = node.decorator_list
+ decos = [d.id for d in decos if isinstance(d, ast.Name)]
+
+ if 'classmethod' in decos:
+ node.function_type = 'classmethod'
+ elif 'staticmethod' in decos:
+ node.function_type = 'staticmethod'
+ else:
+ node.function_type = 'method'
+
+ else:
+ node.function_type = 'method'
+
+
+ def find_global_defs(self, func_def_node):
+ global_names = set()
+ nodes_to_check = deque(ast.iter_child_nodes(func_def_node))
+ while nodes_to_check:
+ node = nodes_to_check.pop()
+ if isinstance(node, ast.Global):
+ global_names.update(node.names)
+
+ if not isinstance(node, (ast.FunctionDef, ast.ClassDef)):
+ nodes_to_check.extend(ast.iter_child_nodes(node))
+ func_def_node.global_names = global_names
+
+
+class ClassNameASTCheck(BaseAstCheck):
+ """
+ Almost without exception, class names use the CapWords convention.
+
+ Classes for internal use have a leading underscore in addition.
+ """
+ CLASS_NAME_RGX = re.compile('[_A-Z][a-zA-Z0-9]*$')
+ text = "E800 class names should use CapWords convention"
+
+ def visit_classdef(self, node, parents):
+ if not self.CLASS_NAME_RGX.match(node.name):
+ self.error_at_node(node, self.text)
+
+
+class FunctionNameASTCheck(BaseAstCheck):
+ """
+ Function names should be lowercase, with words separated by underscores
+ as necessary to improve readability.
+ Functions *not* beeing methods '__' in front and back are not allowed.
+
+ mixedCase is allowed only in contexts where that's already the
+ prevailing style (e.g. threading.py), to retain backwards compatibility.
+ """
+ GOOD_FUNCTION_NAME = re.compile(r"^[_a-z0-9][_a-z0-9]*$")
+ text = "E801 function name does not follow PEP8 guidelines"
+
+ def visit_functiondef(self, node, parents):
+ function_type = getattr(node, 'function_type', 'function')
+ if function_type == 'function':
+ if node.name.startswith('__') or node.name.endswith('__'):
+ self.error_at_node(node, self.text)
+ elif not self.GOOD_FUNCTION_NAME.match(node.name):
+ self.error_at_node(node, self.text)
+ elif not self.GOOD_FUNCTION_NAME.match(node.name):
+ self.error_at_node(node, self.text)
+
+
+class FunctionArgNamesASTCheck(BaseAstCheck):
+ """
+ The argument names of a function should be lowercase, with words separated
+ by underscores.
+
+ A classmethod should have 'cls' as first argument.
+ A method should have 'self' as first argument.
+ """
+
+ GOOD_ARG_NAME = re.compile('[a-z_][a-z0-9_]{0,30}$').match
@florentx Owner

is the 31 chars limit in the PEP 8 recommendations?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ E802 = "E802 argument names do not follow PEP8 guidelines"
+ E803 = "E803 first argument of a classmethod should named 'cls'"
+ E804 = "E804 first argument of a method should named 'self'"
+
+ def visit_functiondef(self, node, parents):
+ if node.args.kwarg is not None:
+ if not self.GOOD_ARG_NAME(node.args.kwarg):
+ self.error_at_node(node, self.E802)
+ return
+
+ if node.args.vararg is not None:
+ if not self.GOOD_ARG_NAME(node.args.vararg):
+ self.error_at_node(node, self.E802)
+ return
+
+ if IS_PY3:
+ arg_names = self._get_arg_names_py3(node)
+ else:
+ arg_names = self._get_arg_names_py2(node)
+
+ function_type = getattr(node, 'function_type', 'function')
+
+ if len(arg_names) > 0:
+ if function_type == 'method':
+ if arg_names[0] != 'self':
+ self.error_at_node(node, self.E804)
+ elif function_type == 'classmethod':
+ if arg_names[0] != 'cls':
+ self.error_at_node(node, self.E803)
+
+ for arg in arg_names:
+ if not self.GOOD_ARG_NAME(arg):
+ self.error_at_node(node, self.E802)
+ return
+
+ def _get_arg_names_py2(self, node):
+ ret = []
+ for arg in node.args.args:
+ if isinstance(arg, ast.Tuple):
+ for t_arg in arg.elts:
+ ret.append(t_arg.id)
+ else:
+ ret.append(arg.id)
+ return ret
+
+
+ def _get_arg_names_py3(self, node):
+ pos_args = [arg.arg for arg in node.args.args]
+ kw_only = [arg.arg for arg in node.args.kwonlyargs]
+ return pos_args + kw_only
+
+
+class ImportAsASTCheck(BaseAstCheck):
+ """
+ Dont change the nameing convention via an import
+ """
+ GLOBAL_NAME = re.compile('[A-Z_][A-Z0-9_]*$').match
+ LOWER_CASE = re.compile('[a-z_][a-z0-9_]*$').match
+ W800 = "W800 Constant imported as non constant"
+ W801 = "W801 Lowercase imported as non lowercase"
+ W802 = "W802 Camelcase imported as lowercase"
+ W803 = "W803 Camelcase imported as constant"
+
+ def visit_importfrom(self, node, parents):
+ for name in node.names:
+ if not name.asname:
+ continue
+ if self.GLOBAL_NAME(name.name):
+ if not self.GLOBAL_NAME(name.asname):
+ self.error_at_node(node, self.W800)
+ elif self.LOWER_CASE(name.name):
+ if not self.LOWER_CASE(name.asname):
+ self.error_at_node(node, self.W801)
+ elif self.LOWER_CASE(name.asname):
+ self.error_at_node(node, self.W802)
+ elif self.GLOBAL_NAME(name.asname):
+ self.error_at_node(node, self.W803)
+
+
+class VariablesInFunctionsASTCheck(BaseAstCheck):
+ """
+ Local variables in functions should be in lowercase
+ """
+ LOWER_CASE = re.compile('[a-z][a-z0-9_]*$').match
+ E805 = "E805 Variables in functions should be lowercase"
+
+ def visit_assign(self, node, parents):
+ parent_func = self.get_parent_function(parents)
+ if parent_func is None:
+ return
+
+ for name in self._get_target_names(node):
+ if name in parent_func.global_names:
+ return
+ if not self.LOWER_CASE(name):
+ self.error_at_node(node, self.E805)
+
+ def _get_target_names(self, node):
+ targets = set()
+ for target in node.targets:
+ if isinstance(target, ast.Name):
+ targets.add(target.id)
+ return targets
+
+
+##############################################################################
# Helper functions
##############################################################################
@@ -1172,6 +1438,7 @@ def __init__(self, filename, lines=None,
self._io_error = None
self._physical_checks = options.physical_checks
self._logical_checks = options.logical_checks
+ self._ast_checks = options.ast_checks
self.max_line_length = options.max_line_length
self.verbose = options.verbose
self.filename = filename
@@ -1309,11 +1576,24 @@ def generate_tokens(self):
self.generate_tokens)
generate_tokens.__doc__ = " Check if the syntax is valid."
+ def run_ast_checks(self):
+ try:
+ tree = ast.parse(''.join(self.lines))
+ except Exception as error:
+ if self.verbose > 0:
+ msg = "Syntax error (%s) in file %s"
+ print(msg % (error, self.filename))
+ else:
+ visitors = [cls(self) for cls in self._ast_checks]
+ runner = VisitorsRunner(visitors)
+ runner.run(tree)
+
def check_all(self, expected=None, line_offset=0):
"""
Run all checks on the input file.
"""
self.report.init_file(self.filename, self.lines, expected, line_offset)
+ self.run_ast_checks()
self.line_number = 0
self.indent_char = None
self.indent_level = 0
@@ -1570,6 +1850,7 @@ def __init__(self, *args, **kwargs):
options.ignore_code = self.ignore_code
options.physical_checks = self.get_checks('physical_line')
options.logical_checks = self.get_checks('logical_line')
+ options.ast_checks = self.get_classes("ASTCheck")
self.init_report()
def init_report(self, reporter=None):
@@ -1650,6 +1931,14 @@ def get_checks(self, argument_name):
checks.append((name, function, args))
return sorted(checks)
+ def get_classes(self, postfix):
+ classes = []
+ for name, obj in globals().items():
+ if inspect.isclass(obj):
+ if name.endswith(postfix):
+ classes.append(obj)
+ return classes
+
def init_tests(pep8style):
"""
@@ -1675,6 +1964,15 @@ def init_tests(pep8style):
def run_tests(filename):
"""Run all the tests from a file."""
lines = readlines(filename) + ['#:\n']
+
+ # Filter out tests which cannot run in the current python version
+ # Mainly for the AST tests. Some syntax is not supported in python2
+ if IS_PY3 and any(IS_PY2_TEST.search(line) for line in lines[:3]):
+ return
+
+ if not IS_PY3 and any(IS_PY3_TEST.search(line) for line in lines[:3]):
+ return
+
line_offset = 0
codes = ['Okay']
testcase = []
View
8 testsuite/E80.py
@@ -0,0 +1,8 @@
+#: E800
+class notok(object):
+ pass
+#: E800
+class Good(object):
+ class notok(object):
+ pass
+ pass
View
54 testsuite/E81.py
@@ -0,0 +1,54 @@
+#: Okay
+def ok():
+ pass
+#: E801
+def __bad():
+ pass
+#: E801
+def bad__():
+ pass
+#: E801
+def __bad__():
+ pass
+#: Okay
+def _ok():
+ pass
+#: Okay
+def ok_ok_ok_ok():
+ pass
+#: Okay
+def _somehow_good():
+ pass
+#: Okay
+def go_od_():
+ pass
+#: Okay
+def _go_od_():
+ pass
+#: E801
+def NotOK():
+ pass
+#: Okay
+def _():
+ pass
+#: Okay
+class Foo(object):
+ def __method(self):
+ pass
+#: Okay
+class Foo(object):
+ def __method__(self):
+ pass
+#: Okay
+class ClassName(object):
+ def __method__(self):
+ pass
+#: E801
+class ClassName(object):
+ def notOk(self):
+ pass
+#: E801
+class ClassName(object):
+ def method(self):
+ def __bad():
+ pass
View
50 testsuite/E82.py
@@ -0,0 +1,50 @@
+#: Okay
+def b1():
+ pass
+#: Okay
+def b2(a):
+ pass
+#: Okay
+def b3(a, b, c, d):
+ pass
+#: Okay
+def b4(a, b, c, *fuck):
+ pass
+#: Okay
+def b5(*fuck):
+ pass
+#: Okay
+def b6(a, b, c, **kwargs):
+ pass
+#: Okay
+def b7(**kwargs):
+ pass
+#: Okay
+def b8(*args, **kwargs):
+ pass
+#: Okay
+def b9(a, b, c, *args, **kwargs):
+ pass
+#: Okay
+def b10(a, b, c=3, d=4, *args, **kwargs):
+ pass
+#: E802
+def b11(**BAD):
+ pass
+#: E802
+def b12(*BAD):
+ pass
+#: E802
+def b13(BAD, *VERYBAD, **EXTRABAD):
+ pass
+#: E802
+def b14(BAD):
+ pass
+#: E802 E802
+class Test(object):
+ def __init__(self, BAD):
+ pass
+
+ @classmethod
+ def test(cls, BAD):
+ pass
View
7 testsuite/E82_py2.py
@@ -0,0 +1,7 @@
+# python2 only
+#: Okay
+def test(a, b, (good, verygood)):
+ pass
+#: E802
+def bad(a, b, (OHH, NOO)):
+ pass
View
13 testsuite/E82_py3.py
@@ -0,0 +1,13 @@
+# python3 only
+#: Okay
+def compare(a, b, *, key=None):
+ pass
+#: E802
+def compare(a, b, *, BAD=None):
+ pass
+#: E802
+def compare(a, b, *VERY, bad=None):
+ pass
+#: E802
+def compare(a, b, *ok, fine=None, **BAD):
+ pass
View
13 testsuite/E83.py
@@ -0,0 +1,13 @@
+#: E803
+class Foo(object):
+ @classmethod
+ def mmm(cls, ads):
+ pass
+
+ @classmethod
+ def bad(self, ads):
+ pass
+
+ @calling()
+ def test(self, ads):
+ pass
View
7 testsuite/E84.py
@@ -0,0 +1,7 @@
+#: E804
+class Foo(object):
+ def good(self, ads):
+ pass
+
+ def bad(ads, self):
+ pass
View
34 testsuite/E85.py
@@ -0,0 +1,34 @@
+#: Okay
+def test():
+ good = 1
+#: Okay
+def test():
+ def test2():
+ good = 1
+#: Okay
+GOOD = 1
+#: Okay
+class Test(object):
+ GOOD = 1
+#: E805
+def test():
+ Bad = 1
+#: E805
+def test():
+ VERY = 2
+#: E805
+def test():
+ def test2():
+ class Foo(object):
+ def test3(self):
+ Bad = 3
+#: Okay
+def good():
+ global Bad
+ Bad = 1
+#: E805
+def bad():
+ global Bad
+
+ def foo():
+ Bad = 1
View
12 testsuite/W80.py
@@ -0,0 +1,12 @@
+#: Okay
+import good as good
+#: Okay
+from mod import good as nice, NICE as GOOD, Camel as Memel
+#: W800
+from mod import GOOD as bad
+#: W801
+from mod import good as Bad
+#: W802
+from mod import CamelCase as noncamle
+#: W803
+from mod import CamelCase as CONSTANT
Something went wrong with that request. Please try again.