Merge ~jslarraz/review-tools:rework-snap-review into review-tools:master

[Jorge Sancho Larraz]

Improve bin/snap-review in preparation for using a common report class shared across all reviews

[Alex Murray (alexmurray)]

LGTM (was it intentional to include the change to reviewtools/sr_lint etc around desktop files here?)

review: Approve

[Jorge Sancho Larraz (jslarraz)]

Yes, it was hehe

Originally, we initialized review classes with `modules.init_main_class` function. That configured the review_type (json or other) in the review class. It was never used (as snap-review don't use `Review.do_report`) but when `common.error` (which uses report_type) is used to exit the review.

Thus, this update to snap-review requires of updating tests/test.sh.expected and the change to reviewtools/sr_lint etc around desktop files was how I tried to manage this change in a consistent way.

diff --git a/bin/snap-review b/bin/snap-review
index 37c9fea..2c26158 100755
--- a/bin/snap-review
+++ b/bin/snap-review
@@ -1,6 +1,6 @@
 #!/usr/bin/python3
 
-from reviewtools import modules
+from reviewtools import modules as modules_mod
 import argparse
 import copy
 import json
@@ -21,33 +21,53 @@ from reviewtools.common import (
 from reviewtools.sr_declaration import validate_json
 
 
-def print_findings(results, description):
-    """
-    Print a summary of the issues found.
-    """
-
-    if not description or not results:
-        return ""
-    print(description)
-    print("".center(len(description), "-"))
-    for key in sorted(results.keys()):
-        print(" - %s" % key)
-        print("\t%s" % results[key]["text"])
-        if "link" in results[key]:
-            print("\t%s" % results[key]["link"])
-
-
-class Results(object):
+class Report(object):
     results = {}
     errors = {}
     warnings = {}
     info = {}
-    rc = 0
 
-    def __init__(self, args):
-        self.args = args
-        self.pkg_fn = self.args.filename
-        self.modules = modules.get_modules()
+    def __init__(self, filename, verbose: bool = False):
+        self.filename = filename
+        self.verbose = verbose
+
+    # review_report[<section>][<result_type>][<review_name>] = <result>
+    #   section: section of the report (one per review module)
+    #   result_type: info, warn, error
+    #   review_name: name of the check (prefixed with self.review_type)
+    #   result: contents of the review
+    #   link: url for more information
+    #   manual_review: force manual review
+    #   override_result_type: prefix results with [<result_type>] and set
+    #     result_type to override_result_type
+    def add_result(
+        self,
+        section,
+        result_type,
+        review_name,
+        result,
+        link=None,
+        manual_review=False,
+    ):
+        """Add result to report"""
+        if section not in self.results:
+            self.results[section] = {
+                "info": {},
+                "warn": {},
+                "error": {},
+            }
+
+        if result_type not in self.results[section]:
+            raise Exception("Invalid result type '%s'" % result_type)
+
+        if review_name not in self.results[section][result_type]:
+            self.results[section][result_type][review_name] = dict()
+
+        self.results[section][result_type][review_name].update(
+            {"text": "%s" % result, "manual_review": manual_review}
+        )
+        if link is not None:
+            self.results[section][result_type][review_name]["link"] = link
 
     def _summarise_results(self):
         for module in self.results:
@@ -55,94 +75,51 @@ class Results(object):
                 self.errors[key] = self.results[module]["error"][key]
             for key in self.results[module]["warn"]:
                 self.warnings[key] = self.results[module]["warn"][key]
-            if self.args.verbose:
+            if self.verbose:
                 for key in self.results[module]["info"]:
                     self.info[key] = self.results[module]["info"][key]
 
-    def complete_report(self):
+    def print_findings(self, results, description):
+        """
+        Print a summary of the issues found.
+        """
+        if not description or not results:
+            return ""
+        print(description)
+        print("".center(len(description), "-"))
+        for key in sorted(results.keys()):
+            print(" - %s" % key)
+            print("\t%s" % results[key]["text"])
+            if "link" in results[key]:
+                print("\t%s" % results[key]["link"])
+
+    def show_results(self, report_type="console"):
         self._summarise_results()
 
-        if self.args.json:
+        if report_type == "json":
             print(
                 json.dumps(
                     self.results, sort_keys=True, indent=2, separators=(",", ": ")
                 )
             )
-        elif self.args.sdk:
+        elif report_type == "sdk":
             for section in sorted(self.results.keys()):
                 output = self.results[section]
                 print("= %s =" % section)
                 print(
                     json.dumps(output, sort_keys=True, indent=2, separators=(",", ": "))
                 )
-                if output["error"] or output["warn"]:
-                    self.rc = 1
         else:
-            print_findings(self.errors, "Errors")
-            print_findings(self.warnings, "Warnings")
-            if self.args.verbose:
-                print_findings(self.info, "Info")
-            if self.rc == 1:
-                print("%s: RUNTIME ERROR" % self.args.filename)
+            self.print_findings(self.errors, "Errors")
+            self.print_findings(self.warnings, "Warnings")
+            if self.verbose:
+                self.print_findings(self.info, "Info")
+            if "runtime-errors" in self.results:
+                print("%s: RUNTIME ERROR" % self.filename)
             elif self.warnings or self.errors:
-                print("%s: FAIL" % self.args.filename)
+                print("%s: FAIL" % self.filename)
             else:
-                print("%s: pass" % self.args.filename)
-        if self.rc == 1:
-            # always exit(1) if there are errors
-            pass
-        elif self.errors:
-            self.rc = 2
-        elif self.warnings:
-            self.rc = 3
-
-    def _run_module_checks(self, module, overrides, report_type):
-        # What we are doing here is basically what all the
-        # ./bin/snap-check-* scripts do as well, so for
-        # example something like:
-        #
-        #     review = sr_lint.SnapLint(sys.argv[1])
-        #     review.do_checks()
-        #     rc = review.do_report()
-        #
-        section = module.replace("sr_", "snap.v2_")
-        try:
-            review = modules.init_main_class(
-                module, self.pkg_fn, overrides=overrides, report_type=report_type
-            )
-
-            if review:
-                review.do_checks()
-                self.results[section] = review.review_report
-                return section
-        except Exception:
-            print("Caught exception (setting rc=1 and continuing):")
-            traceback.print_exc(file=sys.stdout)
-            self.rc = 1
-        return None
-
-    def run_all_checks(self, overrides):
-        if self.args.json:
-            report_type = "json"
-        elif self.args.sdk:
-            # this should probably be "json", but leave as None since it has
-            # always been that way
-            report_type = None
-        else:
-            report_type = "console"
-
-        for module in self.modules:
-            self._run_module_checks(module, overrides, report_type)
-
-    def add_runtime_error(self, args, name, msg):
-        section = "runtime-errors"
-        if section not in self.results:
-            self.results[section] = {}
-        self.results[section] = {
-            "error": {name: {"text": msg, "manual_review": False}},
-            "info": {},
-            "warn": {},
-        }
+                print("%s: pass" % self.filename)
 
 
 def main():
@@ -201,18 +178,13 @@ def main():
             "file '%s' does not exist." % args.filename, output_type=error_output_type
         )
 
-    results = Results(args)
-    if not results.modules:
+    modules = modules_mod.get_modules()
+    if not modules:
         print("No 'reviewtools' modules found.")
         sys.exit(1)
 
-    overrides = None
-    if args.overrides:
-        overrides = json.loads(args.overrides)
-
+    overrides = json.loads(args.overrides) if args.overrides else {}
     if args.plugs:
-        if overrides is None:
-            overrides = {}
         try:
             with open(args.plugs, "r") as plugs_file:
                 overrides["snap_decl_plugs"] = json.loads(plugs_file.read())
@@ -220,8 +192,6 @@ def main():
         except Exception as e:
             error("Failed to read plugs: %s" % e, output_type=error_output_type)
     if args.slots:
-        if overrides is None:
-            overrides = {}
         try:
             with open(args.slots, "r") as slots_file:
                 overrides["snap_decl_slots"] = json.loads(slots_file.read())
@@ -229,16 +199,10 @@ def main():
         except Exception as e:
             error("Failed to read slots: %s" % e, output_type=error_output_type)
     if args.allow_classic:
-        if overrides is None:
-            overrides = {}
         overrides["snap_allow_classic"] = args.allow_classic
     if args.on_store:
-        if overrides is None:
-            overrides = {}
         overrides["snap_on_store"] = args.on_store
     if args.on_brand:
-        if overrides is None:
-            overrides = {}
         overrides["snap_on_brand"] = args.on_brand
 
     # --state-input alone is unsupported
@@ -249,9 +213,6 @@ def main():
         )
 
     if args.state_output:
-        if overrides is None:
-            overrides = {}
-
         if args.state_input:
             # --state-input --state-output indicates we have previous state
             if not os.path.exists(args.state_input):
@@ -280,7 +241,20 @@ def main():
         overrides["state_output"] = copy.deepcopy(overrides["state_input"])
 
     # Run the tests
-    results.run_all_checks(overrides)
+    rc = 0
+    report = Report(args.filename, args.verbose)
+    for module in modules:
+        section = module.replace("sr_", "snap.v2_")
+        try:
+            review_class = modules_mod.find_main_class(module)
+            review = review_class(args.filename, overrides=overrides)
+
+            review.do_checks()
+            report.results[section] = review.review_report
+        except Exception:
+            print("Caught exception (setting rc=1 and continuing):")
+            traceback.print_exc(file=sys.stdout)
+            rc = 1
 
     # Write out our state
     if args.state_output:
@@ -290,8 +264,11 @@ def main():
         try:
             verify_override_state(overrides["state_output"])
         except Exception as e:
-            results.add_runtime_error(
-                args, "state-output-verify", "state-output failed to verify: %s" % e
+            report.add_result(
+                "runtime-errors",
+                "error",
+                "state-output-verify",
+                "state-output failed to verify: %s" % e,
             )
             overrides["state_output"] = copy.deepcopy(overrides["state_input"])
 
@@ -312,14 +289,35 @@ def main():
                 out_f.flush()
                 shutil.copyfile(out_f.name, args.state_output)
         except Exception as e:
-            results.add_runtime_error(
-                args, "state-output-write", "Could not write state output: %s" % e
+            report.add_result(
+                "runtime-errors",
+                "error",
+                "state-output-write",
+                "Could not write state output: %s" % e,
             )
 
     # Show the results of the report
-    results.complete_report()
-
-    sys.exit(results.rc)
+    if args.json:
+        report_type = "json"
+    elif args.sdk:
+        report_type = "sdk"
+        if report.errors or report.warnings:
+            # TODO: harmonize return codes
+            rc = 1
+    else:
+        report_type = "console"
+
+    # Show results updates of report.errors and report.warnings
+    report.show_results(report_type)
+
+    if rc == 1:
+        # always exit(1) if there are errors
+        pass
+    elif report.errors:
+        rc = 2
+    elif report.warnings:
+        rc = 3
+    sys.exit(rc)
 
 
 if __name__ == "__main__":
diff --git a/reviewtools/modules.py b/reviewtools/modules.py
index 9b93f32..e0d56d2 100644
--- a/reviewtools/modules.py
+++ b/reviewtools/modules.py
@@ -83,23 +83,3 @@ def find_main_class(module_name):
         return None
     init_object = getattr(module, test_class[0][0])
     return init_object
-
-
-def init_main_class(module_name, pkg_file, overrides=None, report_type=None):
-    """
-    This function will instantiate the main Snap*Review
-    class of a given module and instantiate it with the
-    location of the file we want to inspect.
-    """
-
-    init_object = find_main_class(module_name)
-    if not init_object:
-        return None
-    try:
-        ob = init_object(pkg_file, overrides)
-        # set the report_type separately since it is in the common class
-        ob.set_report_type(report_type)
-    except TypeError as e:
-        print("Could not init %s: %s" % (init_object, str(e)))
-        raise
-    return ob