From 059d73d5e641af01796e0c39293c1d9a2f617c97 Mon Sep 17 00:00:00 2001 From: sol1105 Date: Wed, 12 Jun 2024 15:18:45 +0200 Subject: [PATCH 1/3] Adding inputs (i.e. options incl. value) - adding command line option -I/--input - adding inputs to CheckSuite, ComplianceChecker and BaseCheck --- cchecker.py | 47 ++++++++++++++++++++++++++++++++++++ compliance_checker/base.py | 6 ++++- compliance_checker/runner.py | 3 ++- compliance_checker/suite.py | 6 +++-- 4 files changed, 58 insertions(+), 4 deletions(-) diff --git a/cchecker.py b/cchecker.py index 4e5ed5d4..ae9eb8c0 100755 --- a/cchecker.py +++ b/cchecker.py @@ -40,6 +40,30 @@ def parse_options(opts): return options_dict +def parse_inputs(inputs): + """ + Helper function to parse possible inputs. Splits input after the first + and second colon to split into checker/key/value triplets. + + :param inputs: Iterable of strings with inputs + :rtype: dict + :return: Dictionary of dictionaries, with first level keys as checker + type (i.e. "cf", "acdd") and second level keys as options. + """ + inputs_dict = {} + for input_str in inputs: + try: + checker_type, checker_opt, checker_val = input_str.split(":", 2) + except ValueError: + warnings.warn(f"Could not split input {input_str}, ignoring", stacklevel=2) + else: + try: + inputs_dict[checker_type].update({checker_opt: checker_val}) + except KeyError: + inputs_dict[checker_type] = {checker_opt: checker_val} + return inputs_dict + + def main(): # Load all available checker classes check_suite = CheckSuite() @@ -185,6 +209,26 @@ def main(): ), ) + parser.add_argument( + "-I", + "--input", + default=[], + action="append", + help=dedent( + """ + Additional input options to be passed to the + checkers. Multiple input options can be specified + via multiple invocations of this switch. + Input options should be prefixed with a the + checker name followed by the input name and the value , e.g. + '::' + + For now this switch exists to support more complex command line options + for plugins. + """, + ), + ) + parser.add_argument( "-V", "--version", @@ -236,6 +280,7 @@ def main(): sys.exit(0) options_dict = parse_options(args.option) if args.option else defaultdict(set) + inputs_dict = parse_inputs(args.input) if args.input else {} if args.describe_checks: error_stat = 0 @@ -306,6 +351,7 @@ def main(): args.output[0], args.format or ["text"], options=options_dict, + inputs=inputs_dict, ) return_values.append(return_value) had_errors.append(errors) @@ -326,6 +372,7 @@ def main(): output, args.format or ["text"], options=options_dict, + inputs=inputs_dict, ) return_values.append(return_value) had_errors.append(errors) diff --git a/compliance_checker/base.py b/compliance_checker/base.py index 4cf5e751..1d59bc4b 100644 --- a/compliance_checker/base.py +++ b/compliance_checker/base.py @@ -154,12 +154,16 @@ def setup(self, ds): Automatically run when running a CheckSuite. Define this method in your Checker class. """ - def __init__(self, options=None): + def __init__(self, options=None, inputs=None): self._defined_results = defaultdict(lambda: defaultdict(dict)) if options is None: self.options = set() else: self.options = options + if inputs is None: + self.inputs = {} + else: + self.inputs = inputs def get_test_ctx(self, severity, name, variable=None): """ diff --git a/compliance_checker/runner.py b/compliance_checker/runner.py index 536888f5..f8df9436 100644 --- a/compliance_checker/runner.py +++ b/compliance_checker/runner.py @@ -42,6 +42,7 @@ def run_checker( output_filename="-", output_format="text", options=None, + inputs=None, ): """ Static check runner. @@ -58,7 +59,7 @@ def run_checker( @returns If the tests failed (based on the criteria) """ all_groups = [] - cs = CheckSuite(options=options or {}) + cs = CheckSuite(options=options or {}, inputs=inputs or {}) # using OrderedDict is important here to preserve the order # of multiple datasets which may be passed in score_dict = OrderedDict() diff --git a/compliance_checker/suite.py b/compliance_checker/suite.py index a548aae7..4a6c9423 100644 --- a/compliance_checker/suite.py +++ b/compliance_checker/suite.py @@ -62,9 +62,10 @@ class CheckSuite: ) # Base dict of checker names to BaseCheck derived types, override this in your CheckSuite implementation templates_root = "compliance_checker" # modify to load alternative Jinja2 templates - def __init__(self, options=None): + def __init__(self, options=None, inputs=None): self.col_width = 40 self.options = options or {} + self.inputs = inputs or {} @classmethod def _get_generator_plugins(cls): @@ -402,10 +403,11 @@ def run_all(self, ds, checker_names, include_checks=None, skip_checks=None): # version baked in checker_type_name = checker_name.split(":")[0] checker_opts = self.options.get(checker_type_name, set()) + checker_inpts = self.inputs.get(checker_type_name, {}) # instantiate a Checker object try: - checker = checker_class(options=checker_opts) + checker = checker_class(options=checker_opts, inputs=checker_inpts) # hacky fix for no options in constructor except TypeError: checker = checker_class() From d5993f761b4591e8344020c301b09245d0c9049a Mon Sep 17 00:00:00 2001 From: sol1105 Date: Fri, 12 Jul 2024 07:16:03 +0200 Subject: [PATCH 2/3] Using option command line argument instead of defining separate input option --- cchecker.py | 70 ++++++----------------------- compliance_checker/base.py | 8 +--- compliance_checker/runner.py | 3 +- compliance_checker/suite.py | 8 ++-- compliance_checker/tests/test_cf.py | 2 +- 5 files changed, 21 insertions(+), 70 deletions(-) diff --git a/cchecker.py b/cchecker.py index ae9eb8c0..1327a3b2 100755 --- a/cchecker.py +++ b/cchecker.py @@ -22,48 +22,28 @@ def _print_checker_name_header(checker_str): def parse_options(opts): """ - Helper function to parse possible options. Splits option after the first - colon to split into key/value pairs. + Helper function to parse possible options. Splits option into key/value + pairs and optionally a value for the checker option. The separator + is a colon. :param opts: Iterable of strings with options :rtype: dict - :return: Dictionary with keys as checker type (i.e. "cf", "acdd") + :return: Dictionary with keys as checker type (i.e. "cf", "acdd"). + Each value is a dictionary where keys are checker options and values + are checker option values or None if not provided. """ - options_dict = defaultdict(set) + options_dict = defaultdict(dict) for opt_str in opts: try: - checker_type, checker_opt = opt_str.split(":", 1) + checker_type, checker_opt, *checker_val = opt_str.split(":", 2) + checker_val = checker_val[0] if checker_val else None except ValueError: warnings.warn(f"Could not split option {opt_str}, ignoring", stacklevel=2) else: - options_dict[checker_type].add(checker_opt) + options_dict[checker_type][checker_opt] = checker_val return options_dict -def parse_inputs(inputs): - """ - Helper function to parse possible inputs. Splits input after the first - and second colon to split into checker/key/value triplets. - - :param inputs: Iterable of strings with inputs - :rtype: dict - :return: Dictionary of dictionaries, with first level keys as checker - type (i.e. "cf", "acdd") and second level keys as options. - """ - inputs_dict = {} - for input_str in inputs: - try: - checker_type, checker_opt, checker_val = input_str.split(":", 2) - except ValueError: - warnings.warn(f"Could not split input {input_str}, ignoring", stacklevel=2) - else: - try: - inputs_dict[checker_type].update({checker_opt: checker_val}) - except KeyError: - inputs_dict[checker_type] = {checker_opt: checker_val} - return inputs_dict - - def main(): # Load all available checker classes check_suite = CheckSuite() @@ -198,8 +178,9 @@ def main(): checkers. Multiple options can be specified via multiple invocations of this switch. Options should be prefixed with a the - checker name followed by the option, e.g. - ':' + checker name followed by the option, + potentially followed by a value, e.g. + ':[:]' Available options: 'cf:enable_appendix_a_checks' - Allow check @@ -209,26 +190,6 @@ def main(): ), ) - parser.add_argument( - "-I", - "--input", - default=[], - action="append", - help=dedent( - """ - Additional input options to be passed to the - checkers. Multiple input options can be specified - via multiple invocations of this switch. - Input options should be prefixed with a the - checker name followed by the input name and the value , e.g. - '::' - - For now this switch exists to support more complex command line options - for plugins. - """, - ), - ) - parser.add_argument( "-V", "--version", @@ -279,8 +240,7 @@ def main(): print(f"IOOS compliance checker version {__version__}") sys.exit(0) - options_dict = parse_options(args.option) if args.option else defaultdict(set) - inputs_dict = parse_inputs(args.input) if args.input else {} + options_dict = parse_options(args.option) if args.option else defaultdict(dict) if args.describe_checks: error_stat = 0 @@ -351,7 +311,6 @@ def main(): args.output[0], args.format or ["text"], options=options_dict, - inputs=inputs_dict, ) return_values.append(return_value) had_errors.append(errors) @@ -372,7 +331,6 @@ def main(): output, args.format or ["text"], options=options_dict, - inputs=inputs_dict, ) return_values.append(return_value) had_errors.append(errors) diff --git a/compliance_checker/base.py b/compliance_checker/base.py index 1d59bc4b..0f276754 100644 --- a/compliance_checker/base.py +++ b/compliance_checker/base.py @@ -154,16 +154,12 @@ def setup(self, ds): Automatically run when running a CheckSuite. Define this method in your Checker class. """ - def __init__(self, options=None, inputs=None): + def __init__(self, options=None): self._defined_results = defaultdict(lambda: defaultdict(dict)) if options is None: - self.options = set() + self.options = {} else: self.options = options - if inputs is None: - self.inputs = {} - else: - self.inputs = inputs def get_test_ctx(self, severity, name, variable=None): """ diff --git a/compliance_checker/runner.py b/compliance_checker/runner.py index f8df9436..536888f5 100644 --- a/compliance_checker/runner.py +++ b/compliance_checker/runner.py @@ -42,7 +42,6 @@ def run_checker( output_filename="-", output_format="text", options=None, - inputs=None, ): """ Static check runner. @@ -59,7 +58,7 @@ def run_checker( @returns If the tests failed (based on the criteria) """ all_groups = [] - cs = CheckSuite(options=options or {}, inputs=inputs or {}) + cs = CheckSuite(options=options or {}) # using OrderedDict is important here to preserve the order # of multiple datasets which may be passed in score_dict = OrderedDict() diff --git a/compliance_checker/suite.py b/compliance_checker/suite.py index 4a6c9423..6b534f5f 100644 --- a/compliance_checker/suite.py +++ b/compliance_checker/suite.py @@ -62,10 +62,9 @@ class CheckSuite: ) # Base dict of checker names to BaseCheck derived types, override this in your CheckSuite implementation templates_root = "compliance_checker" # modify to load alternative Jinja2 templates - def __init__(self, options=None, inputs=None): + def __init__(self, options=None): self.col_width = 40 self.options = options or {} - self.inputs = inputs or {} @classmethod def _get_generator_plugins(cls): @@ -402,12 +401,11 @@ def run_all(self, ds, checker_names, include_checks=None, skip_checks=None): # use some kind of checker object with checker type and # version baked in checker_type_name = checker_name.split(":")[0] - checker_opts = self.options.get(checker_type_name, set()) - checker_inpts = self.inputs.get(checker_type_name, {}) + checker_opts = self.options.get(checker_type_name, {}) # instantiate a Checker object try: - checker = checker_class(options=checker_opts, inputs=checker_inpts) + checker = checker_class(options=checker_opts) # hacky fix for no options in constructor except TypeError: checker = checker_class() diff --git a/compliance_checker/tests/test_cf.py b/compliance_checker/tests/test_cf.py index 3db25d2d..04ca15b2 100644 --- a/compliance_checker/tests/test_cf.py +++ b/compliance_checker/tests/test_cf.py @@ -240,7 +240,7 @@ def test_appendix_a(self): dataset = self.load_dataset(STATIC_FILES["bad_data_type"]) # Ordinarily, options would be specified in the checker constructor, but # we set them manually here so we don't have to monkey patch `setUp` - self.cf.options = {"enable_appendix_a_checks"} + self.cf.options = {"enable_appendix_a_checks": None} new_check = copy.deepcopy(self.cf) self.cf.setup(dataset) aa_results = self.cf.check_appendix_a(dataset) From f28f3214ce001be2ca6012874941738b87108285 Mon Sep 17 00:00:00 2001 From: sol1105 Date: Mon, 15 Jul 2024 10:00:14 +0200 Subject: [PATCH 3/3] Adding test for cchecker.py:parse_options --- compliance_checker/tests/test_cli.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/compliance_checker/tests/test_cli.py b/compliance_checker/tests/test_cli.py index 2788ec25..670e5b27 100644 --- a/compliance_checker/tests/test_cli.py +++ b/compliance_checker/tests/test_cli.py @@ -7,9 +7,12 @@ import json import os import platform +import shutil import subprocess import sys from argparse import Namespace +from collections import defaultdict +from importlib.machinery import SourceFileLoader import pytest @@ -268,3 +271,23 @@ def test_nczarr_pass_through(self, zarr_url): output_format="text", ) assert not errors + + +def test_parse_options(): + """Test the option parser of cchecker.py""" + # Load cchecker.py + cchecker = SourceFileLoader("cchecker", shutil.which("cchecker.py")).load_module() + # Simple test checker_type:checker_opt + opt_dict = cchecker.parse_options(["cf:enable_appendix_a_checks"]) + assert opt_dict == defaultdict(dict, {"cf": {"enable_appendix_a_checks": None}}) + # Test case checker_type:checker_opt:checker_val + opt_dict = cchecker.parse_options( + ["type:opt:val", "type:opt2:val:2", "cf:enable_appendix_a_checks"], + ) + assert opt_dict == defaultdict( + dict, + { + "type": {"opt": "val", "opt2": "val:2"}, + "cf": {"enable_appendix_a_checks": None}, + }, + )