Repository: jameslamb/doppel-cli

Issue:
Comparison <> public method arguments in different order
`doppel-cli` should throw errors for situations like "method 'X' on class 'A' exists in all packages and all instances have the same arguments but the arguments are in a different order"

Discussion:

Patch:
diff --git a/README.md b/README.md
index fc07162..a9f4968 100644
--- a/README.md
+++ b/README.md
@@ -73,7 +73,7 @@ Cool! Let's do some testing! `doppel-test` can be used to compare multiple packa
 doppel-test \
     --files $(pwd)/test_data/python_${PACKAGE}.json,$(pwd)/test_data/r_${PACKAGE}.json \
     | tee out.log \
-    cat
+    | cat
 ```
 
 This will yield something like this:
@@ -84,7 +84,7 @@ Function Count
 +---------------------+----------------+
 |   argparse [python] |   argparse [r] |
 +=====================+================+
-|                   1 |              1 |
+|                   0 |              1 |
 +---------------------+----------------+
 
 
@@ -93,11 +93,13 @@ Function Names
 +-----------------+---------------------+----------------+
 | function_name   | argparse [python]   | argparse [r]   |
 +=================+=====================+================+
-| ngettext        | yes                 | no             |
-+-----------------+---------------------+----------------+
 | ArgumentParser  | no                  | yes            |
 +-----------------+---------------------+----------------+
 
+Function Argument Names
+=======================
+No shared functions.
+
 Class Count
 ===========
 +---------------------+----------------+
@@ -112,26 +114,34 @@ Class Names
 +-------------------------------+---------------------+----------------+
 | class_name                    | argparse [python]   | argparse [r]   |
 +===============================+=====================+================+
-| HelpFormatter                 | yes                 | no             |
+| MetavarTypeHelpFormatter      | yes                 | no             |
 +-------------------------------+---------------------+----------------+
-| Namespace                     | yes                 | no             |
+| ArgumentParser                | yes                 | no             |
 +-------------------------------+---------------------+----------------+
-| RawDescriptionHelpFormatter   | yes                 | no             |
+| FileType                      | yes                 | no             |
 +-------------------------------+---------------------+----------------+
-| ArgumentParser                | yes                 | no             |
+| HelpFormatter                 | yes                 | no             |
 +-------------------------------+---------------------+----------------+
-| MetavarTypeHelpFormatter      | yes                 | no             |
+| RawDescriptionHelpFormatter   | yes                 | no             |
 +-------------------------------+---------------------+----------------+
 | Action                        | yes                 | no             |
 +-------------------------------+---------------------+----------------+
 | ArgumentDefaultsHelpFormatter | yes                 | no             |
 +-------------------------------+---------------------+----------------+
-| FileType                      | yes                 | no             |
+| Namespace                     | yes                 | no             |
 +-------------------------------+---------------------+----------------+
 | RawTextHelpFormatter          | yes                 | no             |
 +-------------------------------+---------------------+----------------+
 
 
+Class Public Methods
+====================
+No shared classes.
+
+Arguments in Class Public Methods
+=================================
+No shared classes.
+
 Test Failures (12)
 ===================
 1. Function 'ngettext()' is not exported by all packages
diff --git a/doppel/bin/analyze.py b/doppel/bin/analyze.py
index 09e0d3b..6fc2323 100755
--- a/doppel/bin/analyze.py
+++ b/doppel/bin/analyze.py
@@ -142,41 +142,25 @@ while len(modules_to_parse) > 0:
                     out['classes'][obj_name]['public_methods'] = {}
 
                     for f in dir(obj):
-
-                        # If attribute is internal, move on.
-                        # This short-circuiting is nice to also avoid having to deal
-                        # with custom stuff like read-only descriptors
-                        # e.g. https://stackoverflow.com/a/24914634
-                        is_private = f.startswith("_")
-                        is_constructor = f == '__init__'
-                        if is_private and not is_constructor:
-                            continue
-
-                        # Check characteristics of this attribute
                         class_member = getattr(obj, f)
                         class_member = _remove_decorators(class_member)
+
                         is_function = isinstance(class_member, types.FunctionType)
+                        is_public = not f.startswith("_")
+                        is_constructor = f == '__init__'
 
                         # Class methods are technically classes, types.FunctionType()
                         # yields false. But we want to treat them as public methods of
                         # a parent class here
                         # h/t https://stackoverflow.com/a/31843829 on the solution
                         is_class_method = False
-                        if not is_function:
-                            _log_info(f"Checking if '{f}' is a class method")
+                        if is_public and not is_function:
                             try:
                                 is_class_method = str(obj) == str(class_member.__self__)
-                                _log_info(f"'{f}' is a class method")
                             except AttributeError:
                                 pass
 
-                        # If ClassA has a class ClassB as a public member,
-                        # that is basically being used as a class method. Treat it
-                        # like that and grab the arguments of its constructor
-                        if is_class_method and inspect.isclass(class_member):
-                            class_member = class_member.__init__
-
-                        if is_function or is_class_method:
+                        if (is_public or is_constructor) and (is_function or is_class_method):
 
                             method_args = _get_arg_names(
                                 class_member,
@@ -190,7 +174,7 @@ while len(modules_to_parse) > 0:
 
                             # If we're dealing with the class constructor, use the
                             # passed-in replacement value
-                            if is_constructor:
+                            if f == '__init__':
                                 f = CONSTRUCTOR_STRING
 
                             out['classes'][obj_name]['public_methods'][f] = {
diff --git a/doppel/reporters.py b/doppel/reporters.py
index 320246d..6b82bed 100644
--- a/doppel/reporters.py
+++ b/doppel/reporters.py
@@ -189,7 +189,6 @@ class SimpleReporter:
         }
         pkg_names = self.pkg_collection.package_names()
         shared_functions = self.pkg_collection.shared_functions()
-        all_functions = self.pkg_collection.all_functions()
 
         # If there are no shared functions, skip
         if len(shared_functions) == 0:
@@ -200,7 +199,10 @@ class SimpleReporter:
         rows = []
 
         for func_name in shared_functions:
-            args = [func_blocks_by_package[p][func_name]['args'] for p in pkg_names]
+            args = [
+                func_blocks_by_package[p][func_name]['args']
+                for p in pkg_names
+            ]
 
             # check 1: same number of arguments?
             same_length = reduce(lambda a, b: len(a) == len(b), args)
@@ -238,7 +240,7 @@ class SimpleReporter:
         # Report output
         stdout.write('\n{} of the {} functions shared across all packages have identical signatures\n\n'.format(
             len([r for r in filter(lambda x: x[1] == 'yes', rows)]),
-            len(all_functions)
+            len(shared_functions)
         ))
 
         out = _OutputTable(headers=headers, rows=rows)
@@ -376,23 +378,73 @@ class SimpleReporter:
         stdout.write("\nArguments in Class Public Methods\n")
         stdout.write("=================================\n")
 
+        shared_classes = self.pkg_collection.shared_classes()
+        if len(shared_classes) == 0:
+            stdout.write('No shared classes.\n')
+            return
+
+        # Initialize the table
+        headers = ['class.method', 'identical api?']
+        rows = []
+
         shared_methods_by_class = self.pkg_collection.shared_methods_by_class()
         for class_name, methods in shared_methods_by_class.items():
             for method_name in methods:
-                all_args = set(self.pkgs[0].public_method_args(class_name, method_name))
-                shared_args = set(self.pkgs[0].public_method_args(class_name, method_name))
-                for pkg in self.pkgs[1:]:
-                    args = pkg.public_method_args(class_name, method_name)
-                    all_args = all_args.union(args)
-                    shared_args = shared_args.intersection(args)
-
-                # report errors
-                non_shared_args = all_args.difference(shared_args)
-                for arg in non_shared_args:
-                    error_txt = "Not all implementations of '{}.{}()' have keyword argument '{}'."
+
+                display_name = "{}.{}()".format(
+                    class_name,
+                    method_name
+                )
+
+                # Generate a list of lists of args
+                args = [
+                    pkg.public_method_args(class_name, method_name)
+                    for pkg in self.pkgs
+                ]
+
+                # check 1: same number of arguments?
+                same_length = reduce(lambda a, b: len(a) == len(b), args)
+                if not same_length:
+                    error_txt = "Public method '{}()' on class '{}' exists in all packages but with differing number of arguments ({})."
                     error_txt = error_txt.format(
+                        method_name,
                         class_name,
+                        ",".join([str(len(a)) for a in args])
+                    )
+                    self.errors.append(DoppelTestError(error_txt))
+                    rows.append([display_name, 'no'])
+                    continue
+
+                # check 2: same set of arguments
+                same_args = reduce(lambda a, b: sorted(a) == sorted(b), args)
+                if not same_args:
+                    error_txt = "Public method '{}()' on class '{}' exists in all packages but some arguments are not shared in all implementations."
+                    error_txt = error_txt.format(
+                        method_name,
+                        class_name
+                    )
+                    self.errors.append(DoppelTestError(error_txt))
+                    rows.append([display_name, 'no'])
+                    continue
+
+                # check 3: same set or arguments and same order
+                same_order = reduce(lambda a, b: a == b, args)
+                if not same_order:
+                    error_txt = "Public method '{}()' on class '{}' exists in all packages but with differing order of keyword arguments."
+                    error_txt = error_txt.format(
                         method_name,
-                        arg
+                        class_name
                     )
                     self.errors.append(DoppelTestError(error_txt))
+                    rows.append([display_name, 'no'])
+                    continue
+
+                # if you get here, we're gucci
+                rows.append([display_name, 'yes'])
+
+        # Report output
+        out = _OutputTable(headers=headers, rows=rows)
+        out.write()
+
+        # Print output
+        stdout.write("\n")


Test patch:
diff --git a/tests/test_reporters.py b/tests/test_reporters.py
index f791b03..01e2221 100644
--- a/tests/test_reporters.py
+++ b/tests/test_reporters.py
@@ -31,6 +31,7 @@ BASE_PACKAGE = {
     "classes": {}
 }
 
+# testing different function stuff
 PACKAGE_WITH_DIFFERENT_ARG_NUMBER = copy.deepcopy(BASE_PACKAGE)
 PACKAGE_WITH_DIFFERENT_ARG_NUMBER['name'] = 'pkg2'
 PACKAGE_WITH_DIFFERENT_ARG_NUMBER['functions']['playback']['args'].append('other')
@@ -43,6 +44,41 @@ PACKAGE_WITH_DIFFERENT_ARG_ORDER = copy.deepcopy(BASE_PACKAGE)
 PACKAGE_WITH_DIFFERENT_ARG_ORDER['name'] = 'pkg2'
 PACKAGE_WITH_DIFFERENT_ARG_ORDER['functions']['playback']['args'] = ['bass', 'bpm']
 
+# testing different public method stuff
+BASE_PACKAGE_WITH_CLASSES = copy.deepcopy(BASE_PACKAGE)
+BASE_PACKAGE_WITH_CLASSES['classes'] = {
+    'WaleFolarin': {
+        'public_methods': {
+            '~~CONSTRUCTOR~~': {
+                'args': [
+                    'x',
+                    'y',
+                    'z'
+                ]
+            },
+            'no_days_off': {
+                'args': [
+                    'days_to_take',
+                    'songs_to_record'
+                ]
+            }
+        }
+    }
+}
+
+PACKAGE_WITH_DIFFERENT_PM_ARG_NUMBER = copy.deepcopy(BASE_PACKAGE_WITH_CLASSES)
+PACKAGE_WITH_DIFFERENT_PM_ARG_NUMBER['name'] = 'pkg2'
+PACKAGE_WITH_DIFFERENT_PM_ARG_NUMBER['classes']['WaleFolarin']['public_methods']['no_days_off']['args'].append('about_nothing')
+
+PACKAGE_WITH_DIFFERENT_PM_ARGS = copy.deepcopy(BASE_PACKAGE_WITH_CLASSES)
+PACKAGE_WITH_DIFFERENT_PM_ARGS['name'] = 'pkg2'
+PACKAGE_WITH_DIFFERENT_PM_ARGS['classes']['WaleFolarin']['public_methods']['no_days_off']['args'] = ['days_to_take', 'outchyea']
+
+PACKAGE_WITH_DIFFERENT_PM_ARG_ORDER = copy.deepcopy(BASE_PACKAGE_WITH_CLASSES)
+PACKAGE_WITH_DIFFERENT_PM_ARG_ORDER['name'] = 'pkg2'
+PACKAGE_WITH_DIFFERENT_PM_ARG_ORDER['classes']['WaleFolarin']['public_methods']['no_days_off']['args'] = ['songs_to_record', 'days_to_take']
+
+# super different
 PACKAGE_SUPER_DIFFERENT = copy.deepcopy(BASE_PACKAGE)
 PACKAGE_SUPER_DIFFERENT['name'] = 'pkg2'
 PACKAGE_SUPER_DIFFERENT['functions']['playback']['args'] = ['stuff', 'things', 'bass']
@@ -179,6 +215,73 @@ class TestSimpleReporter(unittest.TestCase):
             len(errors) == 0,
         )
 
+    def test_public_method_arg_number(self):
+        """
+        SimpleReporter should catch the condition where
+        public method implementations (same method, same class)
+        in two packages have different number of keyword arguments
+        """
+        reporter = SimpleReporter(
+            pkgs=[PackageAPI(BASE_PACKAGE_WITH_CLASSES), PackageAPI(PACKAGE_WITH_DIFFERENT_PM_ARG_NUMBER)],
+            errors_allowed=100
+        )
+        reporter._check_class_public_method_args()
+        errors = reporter.errors
+        self.assertTrue(
+            len(errors) == 1,
+        )
+        self.assertTrue(
+            all([isinstance(x, DoppelTestError) for x in errors])
+        )
+        self.assertTrue(
+            errors[0].msg == "Public method 'no_days_off()' on class 'WaleFolarin' exists in all packages but with differing number of arguments (2,3)."
+        )
+
+    def test_public_method_args(self):
+        """
+        SimpleReporter should catch the condition where
+        public method implementations (same method, same class)
+        in two packages have different different arguments (even if
+        they have the same number of arguments)
+        """
+        reporter = SimpleReporter(
+            pkgs=[PackageAPI(BASE_PACKAGE_WITH_CLASSES), PackageAPI(PACKAGE_WITH_DIFFERENT_PM_ARGS)],
+            errors_allowed=100
+        )
+        reporter._check_class_public_method_args()
+        errors = reporter.errors
+        self.assertTrue(
+            len(errors) == 1,
+        )
+        self.assertTrue(
+            all([isinstance(x, DoppelTestError) for x in errors])
+        )
+        self.assertTrue(
+            errors[0].msg == "Public method 'no_days_off()' on class 'WaleFolarin' exists in all packages but some arguments are not shared in all implementations."
+        )
+
+    def test_public_method_arg_order(self):
+        """
+        SimpleReporter should catch errors of the form
+        'this function has the same keyword args in
+        both packages but they are in different orders'
+        """
+        reporter = SimpleReporter(
+            pkgs=[PackageAPI(BASE_PACKAGE_WITH_CLASSES), PackageAPI(PACKAGE_WITH_DIFFERENT_PM_ARG_ORDER)],
+            errors_allowed=100
+        )
+        reporter._check_class_public_method_args()
+        errors = reporter.errors
+        self.assertTrue(
+            len(errors) == 1,
+        )
+        self.assertTrue(
+            all([isinstance(x, DoppelTestError) for x in errors])
+        )
+        self.assertTrue(
+            errors[0].msg == "Public method 'no_days_off()' on class 'WaleFolarin' exists in all packages but with differing order of keyword arguments."
+        )
+
     def test_totally_empty(self):
         """
         SimpleReporter should be fine if two packages