Merge lp:~dholbach/click-reviewers-tools/modules-module-plus-tests into lp:click-reviewers-tools

[Daniel Holbach]

[Martin Albisetti (beuno)]

105 + lambda a: a.startswith('cr_') and \

I feel like we're so close to not having that logic spread around, on what to filter out, it feels like a bit of a shame to have that there instead of in a global variable as well.

I don't feel strongly enough about it to block it, so here's my +1, and decide what you feel is the best balance.

review: Approve

[Daniel Holbach (dholbach)]

Makes perfect sense.

[lp:~dholbach/click-reviewers-tools/modules-module-plus-tests]

233. By Daniel Holbach on 2014-08-27

move functionality to find relevant modules into its own simple function

234. By Daniel Holbach on 2014-08-27

make sure we actually have clickreviews modules to look at

[Daniel Holbach (dholbach)]

Hope it's better now.

[Martin Albisetti (beuno)]

Thank you, looks great.

=== modified file 'bin/click-review'
--- bin/click-review	2014-08-25 07:43:05 +0000
+++ bin/click-review	2014-08-27 17:21:20 +0000
@@ -1,54 +1,11 @@
 #!/usr/bin/python3
 
+from clickreviews import modules
 import argparse
-import clickreviews
-import imp
-import inspect
 import json
-import pkgutil
 import sys
 
 
-def get_modules():
-    '''
-    Here we have a look at all the modules in the
-    clickreviews package and filter out a few which
-    are not relevant.
-
-    Basically we look at all the ones which are
-    derived from cr_common, where we can later on
-    instantiate a *Review* object and run the
-    necessary checks.
-    '''
-
-    all_modules = [name for _, name, _ in
-                   pkgutil.iter_modules(clickreviews.__path__)]
-    narrow_down_modules = \
-        lambda a: a.startswith('cr_') and \
-        a not in ['cr_common', 'cr_tests', 'cr_skeleton']
-    return list(filter(narrow_down_modules, all_modules))
-
-
-def init_main_class(module_name, click_file):
-    '''
-    This function will find the Click*Review class in
-    the specified module and instantiate it with the
-    location of the .click file we want to inspect.
-    '''
-
-    module = imp.load_source(module_name,
-                             '%s/%s.py' % (clickreviews.__path__[0],
-                                           module_name))
-
-    classes = inspect.getmembers(module, inspect.isclass)
-    find_main_class = lambda a: a[0].startswith('Click') and \
-        not a[0].endswith('Exception') and \
-        a[1].__module__ == module_name
-    main_class = list(filter(find_main_class, classes))
-    init_object = getattr(module, main_class[0][0])
-    return init_object(click_file)
-
-
 def print_findings(which, kind):
     '''
     Print a summary of the issues found.
@@ -116,12 +73,12 @@
     #     rc = review.do_report()
     #
     results = {}
-    modules = get_modules()
-    if not modules:
+    cr_modules = modules.get_modules()
+    if not cr_modules:
         print("No 'clickreviews' modules found.")
         sys.exit(1)
-    for module in modules:
-        review = init_main_class(module, click_file)
+    for module in cr_modules:
+        review = modules.init_main_class(module, click_file)
         review.do_checks()
         results[module.replace('cr_', '')] = review.click_report
     rc = report(results, args)

=== added file 'clickreviews/modules.py'
--- clickreviews/modules.py	1970-01-01 00:00:00 +0000
+++ clickreviews/modules.py	2014-08-27 17:21:20 +0000
@@ -0,0 +1,67 @@
+import clickreviews
+import imp
+import inspect
+import os
+import pkgutil
+
+IRRELEVANT_MODULES = ['cr_common', 'cr_tests', 'cr_skeleton']
+
+
+def narrow_down_modules(modules):
+    '''
+    Get a list of file names or module names and filter out
+    the ones we know are irrelevant.
+    '''
+    relevant_modules = []
+    for module in modules:
+        module_name = os.path.basename(module).replace('.py', '')
+        if module_name not in IRRELEVANT_MODULES and \
+                module_name.startswith('cr_'):
+            relevant_modules += [module]
+    return relevant_modules
+
+
+def get_modules():
+    '''
+    Here we have a look at all the modules in the
+    clickreviews package and filter out a few which
+    are not relevant.
+
+    Basically we look at all the ones which are
+    derived from cr_common, where we can later on
+    instantiate a *Review* object and run the
+    necessary checks.
+    '''
+
+    all_modules = [name for _, name, _ in
+                   pkgutil.iter_modules(clickreviews.__path__)]
+    return narrow_down_modules(all_modules)
+
+
+def find_main_class(module_name):
+    '''
+    This function will find the Click*Review class in
+    the specified module.
+    '''
+    module = imp.load_source(module_name,
+                             '%s/%s.py' % (clickreviews.__path__[0],
+                                           module_name))
+
+    classes = inspect.getmembers(module, inspect.isclass)
+    find_main_class = lambda a: a[0].startswith('Click') and \
+        not a[0].endswith('Exception') and \
+        a[1].__module__ == module_name
+    main_class = list(filter(find_main_class, classes))
+    init_object = getattr(module, main_class[0][0])
+    return init_object
+
+
+def init_main_class(module_name, click_file):
+    '''
+    This function will instantiate the main Click*Review
+    class of a given module and instantiate it with the
+    location of the .click file we want to inspect.
+    '''
+
+    init_object = find_main_class(module_name)
+    return init_object(click_file)

=== added file 'clickreviews/tests/test_modules.py'
--- clickreviews/tests/test_modules.py	1970-01-01 00:00:00 +0000
+++ clickreviews/tests/test_modules.py	2014-08-27 17:21:20 +0000
@@ -0,0 +1,29 @@
+from clickreviews import modules, cr_tests
+import clickreviews
+import glob
+
+
+class TestModules(cr_tests.TestClickReview):
+    '''Tests for the modules module.'''
+    def setUp(self):
+        self.cr_modules = modules.get_modules()
+        cr_tests.mock_patch()
+        super()
+
+    def test_number_of_suitable_modules(self):
+        path = clickreviews.__path__[0]
+        module_files = glob.glob(path + '/*.py')
+        relevant_module_files = modules.narrow_down_modules(module_files)
+        self.assertEqual(len(relevant_module_files),
+                         len(self.cr_modules))
+
+    def test_number_of_suitable_modules_greater0(self):
+        self.assertTrue(len(self.cr_modules) > 0)
+
+    def test_number_of_available_review_classes(self):
+        count = 0
+        for module_name in self.cr_modules:
+            review = modules.find_main_class(module_name)
+            if review:
+                count += 1
+        self.assertEqual(count, len(self.cr_modules))

=== modified file 'debian/changelog'
--- debian/changelog	2014-08-27 14:22:21 +0000
+++ debian/changelog	2014-08-27 17:21:20 +0000
@@ -1,6 +1,7 @@
 click-reviewers-tools (0.10) UNRELEASED; urgency=medium
 
-  * 
+  * Split out code to find Click*Review classes in the clickreviews package
+    into its own module, add tests for it.
 
  -- Daniel Holbach <[e-mail]>  Wed, 27 Aug 2014 16:22:03 +0200