Merge lp:~justin-fathomdb/nova/termie-bot into lp:~hudson-openstack/nova/trunk

[justinsb]

Simple style-checker that can check some of our style rules, and is easy to extend

[Thierry Carrez (ttx)]

I like the idea, but does this need to be shipped in the core code ? It looks a bit developer-only to me.

[justinsb (justin-fathomdb)]

Sorry Thierry, I don't really understand. Are you saying that contrib is
shipped, and so it's in the wrong place? Where should it be if so?

Otherwise, we have lots of code for developers-only - this is like pylintrc
or the unit tests. Eventually, we might be able to make this part of the
pre-merge checks.

[Thierry Carrez (ttx)]

Maybe I'm overzealous in trying to keep the nova core code focused :)

My point is that the termie-bot is generally useful for openstack projects and is not nova-specific, so maybe it could live outside the nova source tree. Like pep8.

That said, I don't really care, so if the nova-core consensus is that it should live in core code, i'm fine with it.

[justinsb (justin-fathomdb)]

I think we have different style rules in nova than in some of the other
projects; let's start this here and see if other projects want to grab and
adapt it to their own styles. We can pull it out in future!

[Jay Pipes (jaypipes)]

89 +class TwoLinesAfterLog(SimpleRegexChecker):
90 + def _build_regex(self):
91 + self.regex = '\nLOG(\s)*=[^\n]*\n[^\n][^\n]'
92 + # We matched a leading \n
93 + self.skip_chars = 1

I know Andy has stated this on your reviews, but this isn't actually part of pep8 nor the HACKING style guide. This:

LOG = logging.getLogger(...)
FLAGS = flags.FLAGS

class Blah():

is perfectly acceptable according to both pep8 and HACKING, as far as I read it. In fact, a brief search through many Nova files shows tons of examples like the above.

-jay

[justinsb (justin-fathomdb)]

Jay - I'm guessing termie would say it comes under this:

"- thou shalt put two newlines twixt toplevel code (funcs, classes, etc)"

But half the point of this is to get consistency by codifying these rules, as well as to let code reviewers focus on higher level concerns (like the pep8 checker does).

And, yes, this checker does indeed flag a large number of issues in the current code.

[Jay Pipes (jaypipes)]

On Mon, Mar 28, 2011 at 12:50 PM, justinsb <email address hidden> wrote:
> Jay - I'm guessing termie would say it comes under this:
>
> "- thou shalt put two newlines twixt toplevel code (funcs, classes, etc)"
>
> But half the point of this is to get consistency by codifying these rules, as well as to let code reviewers focus on higher level concerns (like the pep8 checker does).

If that were what termie meant, I don't think he would have written
half the code in Nova to group top-level class constants together...

Do you REALLY want to see every module-level constant separated by 2 newlines?

-jay

[termie (termie)]

> On Mon, Mar 28, 2011 at 12:50 PM, justinsb <email address hidden> wrote:
> > Jay - I'm guessing termie would say it comes under this:
> >
> > "- thou shalt put two newlines twixt toplevel code (funcs, classes, etc)"
> >
> > But half the point of this is to get consistency by codifying these rules,
> as well as to let code reviewers focus on higher level concerns (like the pep8
> checker does).
>
> If that were what termie meant, I don't think he would have written
> half the code in Nova to group top-level class constants together...
>
> Do you REALLY want to see every module-level constant separated by 2 newlines?
>

Yes, though it is more specifically different types of objects, LOG and FLAGS are pretty different where as FLAGS and flags.DEFINE_boolean are not. We shouldn't actually have many constants, however, as most actual constants are defined as FLAGS which allows them to be described.

I can go either way on LOG specifically, I don't actually like how it is used to begin with, but my interpretation would put two lines between it and FLAGS.

> -jay

[termie (termie)]

> Maybe I'm overzealous in trying to keep the nova core code focused :)
>
> My point is that the termie-bot is generally useful for openstack projects and
> is not nova-specific, so maybe it could live outside the nova source tree.
> Like pep8.
>
> That said, I don't really care, so if the nova-core consensus is that it
> should live in core code, i'm fine with it.

I think the definition of what is "core" will shift greatly after the next summit, so I think contrib or tools is a fine place for this at the moment.

[Jay Pipes (jaypipes)]

On Mon, Mar 28, 2011 at 1:16 PM, termie <email address hidden> wrote:
>> Maybe I'm overzealous in trying to keep the nova core code focused :)
>>
>> My point is that the termie-bot is generally useful for openstack projects and
>> is not nova-specific, so maybe it could live outside the nova source tree.
>> Like pep8.
>>
>> That said, I don't really care, so if the nova-core consensus is that it
>> should live in core code, i'm fine with it.
>
> I think the definition of what is "core" will shift greatly after the next summit, so I think contrib or tools is a fine place for this at the moment.

Sorry, could you explain this? Statements like the above make it seem
like things have been decided by people, and I'm not familiar with
these decisions.

-jay

[Jay Pipes (jaypipes)]

On Mon, Mar 28, 2011 at 1:16 PM, termie <email address hidden> wrote:
>> On Mon, Mar 28, 2011 at 12:50 PM, justinsb <email address hidden> wrote:
>> > Jay - I'm guessing termie would say it comes under this:
>> >
>> > "- thou shalt put two newlines twixt toplevel code (funcs, classes, etc)"
>> >
>> > But half the point of this is to get consistency by codifying these rules,
>> as well as to let code reviewers focus on higher level concerns (like the pep8
>> checker does).
>>
>> If that were what termie meant, I don't think he would have written
>> half the code in Nova to group top-level class constants together...
>>
>> Do you REALLY want to see every module-level constant separated by 2 newlines?
>>
>
> Yes, though it is more specifically different types of objects, LOG and FLAGS are pretty different where as FLAGS and flags.DEFINE_boolean are not. We shouldn't actually have many constants, however, as most actual constants are defined as FLAGS which allows them to be described.
>
> I can go either way on LOG specifically, I don't actually like how it is used to begin with, but my interpretation would put two lines between it and FLAGS.

I agree that separating different "types of objects" is beneficial.
But LOG and FLAGS are both merely aliases for objects somewhere else.
So their pretty much the same thing in my book.

Something to discuss at the summit, I guess, since it's clear from
looking at the Nova code that the HACKING text is interpreted
(ignored?) differently by different devs.

-jay

[termie (termie)]

- copyright and license declarations
- i'd put the CHECKER stuff at the bottom of the file if it needs to be built after stuff. Alternatively you can make a small decorator for the class that automatically adds the class and define the CHECKER global at the top
- you need an if __name__ == '__main__': at the bottom so that people can import this without executing it
- should we make the output look more like pylint?

nova/api/openstack/_id_translator.py:24:19: E261 at least two spaces before inline comment

and it also has the description underneath, though that is an easy extension to make after the fact (just add a method that prints the appropriate rule)

- i can imagine some issues with the 'raise' vs 'raise e' check, while it is usually the case we have some explicit cases right now where we need to 'raise e'
- the docstring for RethrowExceptionsCarefully needs some adjustment :p

review: Needs Fixing

[justinsb (justin-fathomdb)]

>
> Something to discuss at the summit, I guess, since it's clear from
> looking at the Nova code that the HACKING text is interpreted
> (ignored?) differently by different devs.

Absolutely... If pep8 finds a violation, it takes me < 1 minute to fix it.
 With our rules that pep8 doesn't fix, if I have to go through the push /
propose / "needs fixing' review / move to WIP / fix / push / comment / move
to 'Needs Review' process, it probably takes 10 minutes, and that's ignoring
the reviewers time (unnecessary email to nova-core), and the fact that some
of these violations are likely overlooked. The more we can get into the
automated category, the better in my book - that's what I'm trying to
achieve here.

There's a secondary issue which is that we don't yet agree on all the rules.
 When do we get our PTLs? - hopefully they can just make these decisions by
dictat.

[termie (termie)]

> On Mon, Mar 28, 2011 at 1:16 PM, termie <email address hidden> wrote:
> >> Maybe I'm overzealous in trying to keep the nova core code focused :)
> >>
> >> My point is that the termie-bot is generally useful for openstack projects
> and
> >> is not nova-specific, so maybe it could live outside the nova source tree.
> >> Like pep8.
> >>
> >> That said, I don't really care, so if the nova-core consensus is that it
> >> should live in core code, i'm fine with it.
> >
> > I think the definition of what is "core" will shift greatly after the next
> summit, so I think contrib or tools is a fine place for this at the moment.
>
> Sorry, could you explain this? Statements like the above make it seem
> like things have been decided by people, and I'm not familiar with
> these decisions.
>

Just talking about a push for openstack-common, which will probably start separating out what is considered nova core, what is openstack common (not required to be used by all but available to all) and what are things built on top of those libraries. Where this stuff ends up after that discussion is up for grabs, either it will remain here or move somewhere else, but here is a good enough place until the discussion occurs.

> -jay

[Brian Lamar (blamar)]

Does this validate for itself, out of curiosity?

review: Needs Information

[justinsb (justin-fathomdb)]

I thought it did, but I can't remember if I ran it on the final version.
Obviously it should :-)

[justinsb (justin-fathomdb)]

I think this is looking pretty good for a v1:

I added a docstring checker to check some of our rules on docstrings

I added a format flag so that we can output in 'pretty', 'pep8', or 'pylint' style. Pylint should be parseable by the Violations Jenkins plugin (hopefully!).

I left in the RethrowExceptionsCarefully rule, because it seems to catch some real issues. We may have to use the trick that the glance guys found to rethrow an exception. We should try not to lose the stack traces if we're going to be able to support this in production.

[justinsb (justin-fathomdb)]

termie: Sorry missed one of your review points. I don't think I really understand this one?

> - i'd put the CHECKER stuff at the bottom of the file if it needs to be built after stuff. Alternatively you can make a small decorator for the class that automatically adds the class and define the CHECKER global at the top

I though Python defined things top-to-bottom, so I think CHECKER_BASE_CLASSES does work. Is your concern that the position is fragile?

[termie (termie)]

we discussed this a bit in irc, any further thoughts on python-vs-java philosophy?

[justinsb (justin-fathomdb)]

I think the Java philosophy is better in this case, and you think the
opposite! Let's ask the PTL mediate this one. If we want it before the
result, let's just get Vish to make the call.

[justinsb (justin-fathomdb)]

Vish - I think we need your help as PTL. The barrier is a difference of opinion between termie and myself over how to discover classes that perform checks.

I want to find checks the "Java way", using typing. We create a checker by deriving from Checker, perhaps indirectly. If we want to know if a class is a checker, we check the type.

Termie wants to be more "Pythonic". A Checker simply implements the correct method e.g. run_check. We catch the missing-method exception and assume that the class isn't a checker.

I feel that the Pythonic way doesn't get us very much, but we have to give up help from our compiler & toolset. For example, if we were to rename the 'run_check' method, we'd need to change every Checker. Or a Checker implementation might fat-finger the method name. In both cases, the Java approach gives us lots of help - runtime & static analysis checkers - while the Pythonic approach offers us no help at all. I just don't see what we get in return.

In general, this is what we see throughout the code base, where we don't have a well-defined contract, so different implementations don't work homogeneously and we have lots of bugs that we wouldn't have if we allowed our toolset to help us.

[Vish Ishaya (vishvananda)]

I have to say that I find all the logic around BASE_CLASSES a bit ugly. I don't really see the disadvantage of switching to pythonic style here. You can still make all of them subclasses, but it seems a lot easier to go:

try:
   obj.do_check()
except NotImplemented, AttributeError:
   pass

then doing all of the type checking. Getting a list of candidate classes i am less concerned with, but the global variable with skipped classes is a little odd. Since classes could be defined in other files, one possibility:

flags.DEFINE_list('checkers', ['xxx','yyy'], ....)
for checker in checkers:
   obj = utils.import_object(checker) # or equivalent

I'm a big fan of using inheritance for polymorphism and shared code, but basing runtime checks on class information should be avoided where possible.

[justinsb (justin-fathomdb)]

OK, your call as PTL. I hate any code that hides exceptions, but it's
not me that has to support it...

I'll make it Pythonic.

[Brian Lamar (blamar)]

I agree that hiding errors are bad, but we can just as easily replace that 'pass' with an informative log statement or relevant raise if needed. I'm pretty sure Vish was just trying to show the general form.

[justinsb (justin-fathomdb)]

The whole point is that we can't do anything other than swallow the
exception, because we don't know if the class is supposed to be a
Checker or not...

[Brian Lamar (blamar)]

review: Abstain

[Vish Ishaya (vishvananda)]

Only 2000+ termie violations!

review: Approve

[Todd Willey (xtoddx)]

I'd like to have it bitch at me if I don't give it paths. Other than that I think it is good to go.

Unmerged revisions

911. By justinsb on 2011-05-20

Made discovery of checkers Pythonic

910. By justinsb on 2011-05-20

Merged with trunk

909. By justinsb on 2011-03-29

Remove regex for finding docstrings, replace with "my first parser" level code.

The regex was hanging on certain inputs, and also got confused by embedded quotes in docstrings

908. By justinsb on 2011-03-29

Fixed the checker to be checker-compliant

907. By justinsb on 2011-03-28

Code style cleanups

906. By justinsb on 2011-03-28

pep8 fixes

905. By justinsb on 2011-03-28

Check that multi-line docstrings end in a blank line

904. By justinsb on 2011-03-28

Fix pep8

903. By justinsb on 2011-03-28

Basic docstring checking

902. By justinsb on 2011-03-28

Added pep8, pylint, pretty output formats

=== added directory 'contrib/rules'
=== added file 'contrib/rules/do_checks.py'
--- contrib/rules/do_checks.py	1970-01-01 00:00:00 +0000
+++ contrib/rules/do_checks.py	2011-05-20 05:15:12 +0000
@@ -0,0 +1,329 @@
+#!/usr/bin/env python
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 Justin Santa Barbara
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+
+import gflags
+import imp
+import os
+import re
+import sys
+
+import __main__
+
+
+FLAGS = gflags.FLAGS
+gflags.DEFINE_enum('format', 'pretty', ['pretty', 'pylint', 'pep8'],
+                   'Output format to use')
+
+
+class SourceFile(object):
+    """A source code file."""
+
+    def __init__(self, path):
+        self.path = path
+
+        f = open(path, 'r')
+        code = f.read()
+        code = code.replace('\r\n', '\n')
+        code = code.replace('\n\r', '\n')
+        self.code = code
+        self.lines = code.splitlines()
+        f.close()
+        self.module = None
+
+    def get_module(self):
+        # NOTE(justinsb): This would be nice, but causes errors because the
+        # code is actually executed (e.g. flags)
+        if self.module is None:
+            mod_name, _ext = os.path.splitext(os.path.split(self.path)[-1])
+            self.module = imp.load_source(mod_name, self.path)
+        return self.module
+
+    def map_to_line_index(self, index):
+        line_index = 0
+        for i in xrange(index):
+            if self.code[i] == '\n':
+                line_index = line_index + 1
+        return line_index
+
+
+class Violation(object):
+    """Records a rule infraction."""
+
+    def __init__(self, src, index, tag, message):
+        self.src = src
+        self.index = index
+        self.tag = tag
+        self.message = message
+
+    def print_warning(self):
+        if FLAGS.format == 'pylint':
+            self.print_pylint()
+        elif FLAGS.format == 'pep8':
+            self.print_pep8()
+        else:
+            self.print_pretty()
+
+    def print_pylint(self):
+        line = self.src.map_to_line_index(self.index)
+        level = 'C'
+        description = self.tag + ' ' + self.message
+        print "%s:%s: [%s] %s" % (self.src.path, line + 1, level, description)
+
+    def print_pep8(self):
+        line = self.src.map_to_line_index(self.index)
+        level = 'E'
+        column = 1
+        errnum = self.tag
+        description = self.message
+        print ("%s:%s:%s: %s%s %s" %
+               (self.src.path, line + 1, column, level, errnum, description))
+
+    def print_pretty(self):
+        line = self.src.map_to_line_index(self.index)
+        print "%s %s %s" % (self.tag, self.src.path, line + 1)
+        if self.message != self.tag:
+            print "\t%s" % (self.message)
+        for i in xrange(line - 2, line + 3):
+            if i < 0 or i >= len(self.src.lines):
+                continue
+            if i == line:
+                prefix = "*   "
+            else:
+                prefix = "    "
+            print prefix + self.src.lines[i]
+
+
+class Checker(object):
+    """Base class for all rule-checkers."""
+
+    def __init__(self, src):
+        self.src = src
+
+    def do_check(self):
+        raise NotImplementedError()
+
+    def _build_error(self, index, message=None):
+        tag = self.__class__.__name__
+
+        if message is None:
+            message = self.__class__.__doc__
+            if message is None:
+                message = tag
+        return Violation(self.src, index, tag, message)
+
+
+class DocStringChecker(Checker):
+    """Checks docstrings against our style."""
+
+    def do_check(self):
+        errors = []
+
+        # NOTE(justinsb): We hit a bug in re module: the below regex hangs
+        # on some inputs.  In addition, it gets confused by quotes in docs.
+        #regex = ':(\s)*"""((\s|[^"])*?)"""'
+        #matches = re.finditer(regex, self.src.code)
+
+        code = self.src.code
+        start_pos = 0
+
+        while True:
+            docstring_start = code.find('"""', start_pos)
+            if docstring_start == -1:
+                break
+
+            docstring_end = code.find('"""', docstring_start + 3)
+            if docstring_end == -1:
+                break
+            docstring_end += 3
+            docstring = code[docstring_start:docstring_end]
+
+            errors += self._check_docstring(docstring_start, docstring)
+
+            start_pos = docstring_end + 3
+
+        return errors
+
+    def _check_docstring(self, index, text):
+        errors = []
+        assert text.startswith('"""')
+        assert text.endswith('"""')
+        text = text[3:]
+        text = text[:-3]
+
+        lines = text.splitlines()
+
+        if len(lines) == 0:
+            message = "Docstrings should not be empty"
+            errors.append(self._build_error(index, message=message))
+
+        else:
+            if lines[0].startswith(' '):
+                message = "Docstrings do not start with whitespace"
+                errors.append(self._build_error(index, message=message))
+
+            if lines[0].endswith(' '):
+                message = "Docstrings should not end with whitespace"
+                errors.append(self._build_error(index, message=message))
+
+        if len(lines) == 1:
+            # Single line docstring
+            if not lines[0].endswith('.'):
+                message = "A single line docstring ends in a full stop (.)"
+                errors.append(self._build_error(index, message=message))
+
+        if len(lines) > 1:
+            # Multi-line docstring
+            if not lines[0].endswith('.'):
+                message = ("The first line of a multi-line docstring ends in "
+                           "a full stop (.)")
+                errors.append(self._build_error(index, message=message))
+
+            if lines[1]:
+                message = ("The second line of a multi-line docstring must "
+                           "be blank")
+                errors.append(self._build_error(index, message=message))
+
+            if lines[-2] or lines[-1].strip():
+                message = ("Leave a blank line at the end of a multi-line "
+                           "docstring")
+                errors.append(self._build_error(index, message=message))
+
+        # TODO(justinsb): On a class docstring, there should be a blank line
+        # after the closing """
+
+        return errors
+
+
+class SimpleRegexChecker(Checker):
+    """Base class for checkers that find violations based on regex."""
+
+    def __init__(self, src, regex=None, skip_chars=0):
+        super(SimpleRegexChecker, self).__init__(src)
+        self.regex = regex
+        self.skip_chars = skip_chars
+
+    def _build_regex(self):
+        raise NotImplementedError()
+
+    def do_check(self):
+        if not self.regex:
+            self._build_regex()
+
+        errors = []
+        matches = re.finditer(self.regex, self.src.code)
+        for match in matches:
+            index = match.start()
+
+            index = index + self.skip_chars
+
+            errors.append(self._build_error(index))
+
+        return errors
+
+
+class TwoLinesAfterLog(SimpleRegexChecker):
+    """After a LOG statement, leave two blank lines before the FLAGS/code."""
+
+    def _build_regex(self):
+        self.regex = '\nLOG(\s)*=[^\n]*\n[^\n][^\n]'
+        # We matched a leading \n
+        self.skip_chars = 1
+
+
+class RethrowExceptionsCarefully(SimpleRegexChecker):
+    """When rethrowing an exception, do 'raise', not 'raise e'.
+
+    This preserves the stack trace.  Note that there are some complexities
+    with eventlet, because eventlet can wipe the 'current' exception.
+
+    """
+
+    def _build_regex(self):
+        self.regex = '\n[ ]*raise([ ]+)([a-z]+)\n'
+        self.skip_chars = 1
+
+
+class NoWhiteSpaceLeadingDocString(SimpleRegexChecker):
+    """Don't start your docstrings with whitespace."""
+
+    def _build_regex(self):
+        self.regex = '\):\n(\s)*"""\s'
+        self.skip_chars = 3
+
+
+class AvoidAggressivePunctuation(SimpleRegexChecker):
+    """Punctuation that includes repeated question marks can upset people."""
+
+    def _build_regex(self):
+        self.regex = '\?\?'
+
+
+class OneSpaceInTodoOrNotes(SimpleRegexChecker):
+    """Use one space in your TODO and NOTE comments."""
+
+    def _build_regex(self):
+        self.regex = '#(TODO|NOTE)'
+
+
+class ClaimYourTodoAndNotes(SimpleRegexChecker):
+    """Put your name in brackets after a TODO/NOTE."""
+
+    def _build_regex(self):
+        self.regex = '(TODO|NOTE)(\s*):'
+
+
+def check_file(src_path):
+    src = SourceFile(src_path)
+    
+    for x in dir(__main__):
+        s = getattr(__main__, x)
+        if not hasattr(s, 'do_check'):
+            continue
+
+        try:
+            checker = s(src)
+            errors = checker.do_check()
+            for error in errors:
+                error.print_warning()
+        except NotImplementedError, AttributeError:
+            pass
+
+
+def check_dir(dir_path):
+    for child_name in os.listdir(dir_path):
+        child_path = os.path.join(dir_path, child_name)
+        if os.path.isfile(child_path):
+            if child_path.endswith(".py"):
+                check_file(child_path)
+        else:
+            check_dir(child_path)
+
+
+def do_main():
+    args = FLAGS(sys.argv)
+
+    for path in args[1:]:
+        if os.path.isdir(path):
+            check_dir(path)
+        else:
+            check_file(path)
+
+
+if __name__ == '__main__':
+    do_main()