Refactor and fix up recursive resolve and processing functions #126
Conversation
achilleas-k
force-pushed
the
transform-all-the-things
branch
from
2e95a1e
to
18d9cfa
Compare
|
I just realised that the only failing test now is the |
achilleas-k
force-pushed
the
transform-all-the-things
branch
3 times, most recently
from
f4f56e1
to
e734408
Compare
Feel free to drop it in this PR if that turns it green; we decided in #112 to do so. It only needs the removal of the check in |
achilleas-k
changed the title
achilleas-k
changed the title
|
Disabled test (without removing it) and marked as ready. |
Some of the functions in directive.py would will require resolving values recursively. For example, include() should run the loaded tree through the top-level resolve() function to resolve the whole content of the included file recursively. Merge the two modules so that we don't end up with circular imports.
supakeen
force-pushed
the
transform-all-the-things
branch
from
fdb5a2e
to
1ce16a3
Compare
This commit adds a new `traversal.State` that is part of the `resolve()` recursion. It is needed to keep (recursive) state about what state the parsing is in. Currently this is used to keep track of what include is used so that nested includes work and also so that errors can know in what file the issue happened.
achilleas-k
force-pushed
the
transform-all-the-things
branch
from
1ce16a3
to
c1e3c76
Compare
There was a problem hiding this comment.
Thank you! This is very nice indeed - I have a bunch of small nitpicks/ideas/suggestions but if we want to move fast we can ignore them all and just merge as is (I went over this commit by commit so some comments are just related to the commit order/granularity).
| continue | ||
|
|
||
| if key.startswith("otk.include"): | ||
| # TODO: disallow this (see https://github.com/osbuild/otk/issues/116) |
There was a problem hiding this comment.
I guess the plan here is that we introduce it and then revert in a single commit so that (if we actually change our mind) we can get it back trivially ? Did I get this right?
There was a problem hiding this comment.
Yes, mostly. But also my thinking was that this is a spec change that came from a discussion while the implementation was happening. I wanted to isolate the change so that it has more visibility and stands on its own (e.g. a PR that only changes this, linked from the issue, that can be read on its own).
Also, I wasn't sure at the time if it needed some state tracking (something to tell us that we're in a defines block). Now I realise since defines can't branch out to any other directives (except other defines), it's not really necessary. But we should also think about what happens with other directives in general.
| # Replace any variables in a value immediately before doing anything | ||
| # else, so that variables defined in strings are considered in the | ||
| # processing of all directives. | ||
| if isinstance(val, str): |
There was a problem hiding this comment.
I wonder if a test for this could be easily extracted from the example given in the commit message for this change?
There was a problem hiding this comment.
One of the example tests covers this (09-include-with-var). Or would you rather also have a small unit test for it?
achilleas-k
force-pushed
the
transform-all-the-things
branch
from
c1e3c76
to
815d989
Compare
The defines property of the State will be used to keep track of subblocks of variable definitions when processing otk.define directives. The copy() method creates a new State with replaced or copied properties. This will enable us to create State objects with references to sub-objects of the global defines variables. While processing defines sub-objects, the State's defines can be written to to define values nested under the key being processed, while using the global Context object to resolve variable names inside defines.
Doesn't do anything for now, but we pass it around because we're going to need it. Co-authored-by: Michael Vogt <michael.vogt@gmail.com>
This might be reverted if we can find a nicer way of handling it, but it will be very convenient to have direct access to the defines dictionary.
The new process_defines() function handles defines recursively with the
following properties:
- An otk.define nested under another otk.define is ignored (as if the
directive was omitted) and the defines below it are processed
normally.
- Defines are set in the global context immediately, meaning that a
variable can be referenced as soon as it's set. For example, the
following is valid:
otk.define.ones:
one: 1
eins: "${one}" # sets eins = 1
- Variable references in values are resolved immediately, before being
set on the context.
- Subblocks are supported and variables are added and available via
their global name while blocks are processed.
- Variable references are always resolved from the global context. For
example:
otk.define.nested:
alpha: 1
subblock:
alpha: 2
a1: "${alpha}" # = 1, from the global context
a2: "${subblock.alpha}" # = 2
- Includes are resolved during processing of the defines block.
- This is subject to change.
- Object (dictionary) values under keys that are already defined and
are also objects (dictionaries) are merged. For example, the
following:
otk.define.multi_1:
subblock:
alpha: 1
beta: 2
otk.define.multi_2:
subblock:
gamma: 3
results in {"subblock": {"alpha": 1, "beta": 2, "gamma": 3}}
The process_defines() function has no return value. It updates the
subblock directly, and the global ctx.defines indirectly as soon as it
finds a value to set. This is convenient for variable reference lookup
during processing.
Add tests for process_defines():
Replace define() tests and move them to the test_transform module since
we moved the process_defines() function to the transform module now.
Since the process_defines() function doesn't return anything, we use the
ctx.defines dictionary to evaluate the test.
The new process_include() function loads a yaml file and immediately runs it through resolve before returning it, processing directives and variable references as they are found. The path to the file is resolved relative to the file that included it.
Replace any variables in a string value immediately before doing
anything else, so that the resolved values are used in the processing of
all directives. This adds support for specifying a directive with a
string value that can contain variables, such as:
otk.include: "${distro}-vars.yaml"
- Remove elses and elifs when not necessary to make the linter happy. No need for else after return or continue. - Use process_defines() instead of resolve() when a define directive is found. The function updates the global context so there is no need to modify the tree after calling it. - Change the generic Exception() for siblings to a KeyError(). - Move the processing of the external directive above the sibling check. Externals can now have siblings. - When processing an include, copy the tree, remove the directive, and update the copy of the tree before returning it. This is necessary otherwise the resulting tree will still contain the include directive.
Add a test that produces an error with an expected message. For now it only tests that a file not found error includes the name of the source file in the message.
achilleas-k
force-pushed
the
transform-all-the-things
branch
from
815d989
to
3ecb63f
Compare
This PR refactors and fixes the recursive resolve functionality to match the spec. All but one of the examples in
test/data/base/pass successfully. The05-target-in-include.yamlexample fails because the_process()function incommand.pyexpects a target in the entrypoint and errors if it doesn't find one.Commit messages should tell the whole story, but let me highlight a few things here I think are the most important:
ParserState.definesreferences the sub-block being processed, so variable definitions can be added to that sub-block directly. It will also, eventually, probably carry a stack of directives being processed so we can do things like disallow includes under defines.process_include()function loads a yaml file and sends it throughresolve()before returning the contents. I think this should eventually be the entrypoint for the recursion. In other words, the path to the entrypoint yaml file specified on the command line can be sent directly through toprocess_include()and it should all work out.resolve()recursion, then print the target that matches the one specified on the command line (or the default if there's only one). Before printing we would have a tree that contains everything we loaded and resolved, so we could print the flattened document with all targets included and variables resolved and we could also run some validation over it.process_defines()is different from all the other ones in that it modifies the context in-place and doesn't return anything. I wonder if things would be simpler if every resolver/process function worked this way and modified the tree in-place.