From 86a81846055751578fd1ea3bb29a020c04ff431a Mon Sep 17 00:00:00 2001 From: Simon de Vlieger Date: Fri, 19 Apr 2024 07:47:25 +0200 Subject: [PATCH 1/3] context: allow redefinitions of variables Allows redefinitions of variables by default, a command line option is added to warn if this happens. The context can be configured to disallow redefinitions. --- src/otk/command.py | 27 +++++++++++++++++++++++---- src/otk/transform/context.py | 33 ++++++++++++++++++++++++++------- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/src/otk/command.py b/src/otk/command.py index e55ce681..c1afa990 100644 --- a/src/otk/command.py +++ b/src/otk/command.py @@ -35,10 +35,22 @@ "--identifier", help="An identifier to include in all log records generated by `otk -j`. Can only be used together with `-j`.", ) -def root(verbose: int, json: bool, identifier: str) -> None: +@click.option( + "-w", + "--warn", + type=click.Choice(["duplicate-definition"]), + multiple=True, + help="Enable warnings, can be passed multiple times.", +) +@click.pass_context +def root( + ctx: click.Context, verbose: int, json: bool, identifier: str, warn: list[str] +) -> None: """`otk` is the omnifest toolkit. A program to work with omnifest inputs and translate them into the native formats for image build tooling.""" + ctx.ensure_object(dict) + logging.basicConfig( level=logging.WARNING - (10 * verbose), handlers=[ @@ -54,6 +66,11 @@ def root(verbose: int, json: bool, identifier: str) -> None: ], ) + # This context is passed along to all other subcommands. + ctx.obj["context"] = { + "duplicate_definitions_warning": "duplicate-definition" in warn, + } + # We do this check *after* setting up the handlers so the error is formatted if identifier and not json: log.error("cannot use `-i` without also using `-j`") @@ -63,7 +80,8 @@ def root(verbose: int, json: bool, identifier: str) -> None: @root.command() @click.argument("input", type=click.Path(exists=True)) @click.argument("output", type=click.Path(), required=False) -def compile(input: str, output: str | None) -> None: +@click.pass_context +def compile(ctx: click.Context, input: str, output: str | None) -> None: """Compile a given omnifest into its targets.""" log.info("Compiling the input file %r to %r", input, output) @@ -72,7 +90,8 @@ def compile(input: str, output: str | None) -> None: @root.command() @click.argument("input", type=click.Path(exists=True)) @click.argument("output", type=click.Path(), required=False) -def flatten(input: str, output: str | None) -> None: +@click.pass_context +def flatten(ctx: click.Context, input: str, output: str | None) -> None: """Flatten a given omnifest by performing all includes.""" log.info("flattening %r to %r", input, output or "STDOUT") @@ -83,7 +102,7 @@ def flatten(input: str, output: str | None) -> None: # TODO posixpath serializer for json output log.info("include root is %r", str(root)) - context = Context(root) + context = Context(root, **ctx.obj["context"]) tree = Omnifest.from_yaml_path(file).to_tree() diff --git a/src/otk/transform/context.py b/src/otk/transform/context.py index 09b0e792..76bb9b5d 100644 --- a/src/otk/transform/context.py +++ b/src/otk/transform/context.py @@ -20,15 +20,27 @@ class Context: + duplicate_definitions_allowed: bool + duplicate_definitions_warning: bool + _version: int | None _path: pathlib.Path _variables: dict[str, Any] - def __init__(self, path: pathlib.Path | None = None) -> None: + def __init__( + self, + path: pathlib.Path | None = None, + *, + duplicate_definitions_allowed: bool = True, + duplicate_definitions_warning: bool = False, + ) -> None: self._version = None self._path = path if path else pathlib.Path(".") self._variables = {} + self.duplicate_definitions_allowed = duplicate_definitions_allowed + self.duplicate_definitions_warning = duplicate_definitions_warning + def version(self, v: int) -> None: # Set the context version, duplicate definitions with different # versions are an error @@ -40,12 +52,19 @@ def version(self, v: int) -> None: self._version = v def define(self, name: str, value: Any) -> None: - # Since we go through the tree multiple times it's easy to ignore - # duplicate definitions as long as they define to the *same* value. - if name in self._variables and self._variables[name] != value: - raise TransformDefineDuplicateError() - - self._variables[name] = value + if name in self._variables: + if not self.duplicate_definitions_allowed: + raise TransformDefineDuplicateError() + + if self.duplicate_definitions_warning: + log.warn( + "redefinition of %r, previous values was %r and new value is %r", + name, + self._variables[name], + value, + ) + else: + self._variables[name] = value def variable(self, name: str) -> Any: parts = name.split(".") From b451db3ad7a647a8647fcbac7791a0cbafbad0c6 Mon Sep 17 00:00:00 2001 From: Simon de Vlieger Date: Fri, 19 Apr 2024 07:50:49 +0200 Subject: [PATCH 2/3] doc: document redefinition behavior --- doc/directives.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/doc/directives.md b/doc/directives.md index 521796ef..ac2bbb49 100644 --- a/doc/directives.md +++ b/doc/directives.md @@ -12,10 +12,9 @@ other parts of the omnifest. Variable scope is global, an `otk.define` directive anywhere in the omnifest tree will result in the defined names being hoisted to the global scope. -Redefinitions of variables are forbidden and will cause an error when detected. -A redefinition of a variable is assigning a value different from the value it -is currently holding. It is thus wise to 'namespace' variables by putting them -inside a map. +Redefinitions of variables are allowed. This allows for setting up default +values. If `-w duplicate-definition` is passed as an argument to `otk` then +`otk` will warn on all duplicate definitions. Expects a `map` for its value. From 41e4fa7b92295369ed3471c775506f053a1df75f Mon Sep 17 00:00:00 2001 From: Simon de Vlieger Date: Fri, 19 Apr 2024 07:56:58 +0200 Subject: [PATCH 3/3] test: context test for duplicate definitions --- test/test_transform_context.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/test_transform_context.py b/test/test_transform_context.py index c85e7244..b4718398 100644 --- a/test/test_transform_context.py +++ b/test/test_transform_context.py @@ -6,6 +6,7 @@ TransformVariableTypeError, TransformVariableIndexTypeError, TransformVariableIndexRangeError, + TransformDefineDuplicateError, ) @@ -63,3 +64,19 @@ def test_context_unhappy(): with pytest.raises(TransformVariableIndexRangeError): ctx.variable("bar.3") + + +def test_context_duplicate_definition(): + ctx0 = Context() + + # Redefinition allowed + ctx0.define("foo", "bar") + ctx0.define("foo", "bar") + + ctx1 = Context(duplicate_definitions_allowed=False) + + # Redefinition NOT allowed + ctx1.define("foo", "bar") + + with pytest.raises(TransformDefineDuplicateError): + ctx1.define("foo", "bar")