-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement a better Context class #97
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got curious about this PR when I saw it on my GH notifications, so came here just to take a peek. Left some random comments 👍
src/scriptengine/context.py
Outdated
"""ScriptEngine Context class | ||
This is basically a dict that implements | ||
(1) dotted keys (i.e. c['foo.bar'] is equivalent to c['foo']['bar']) | ||
(2) deep merges (via deepmerge.always_merger) available as '.merge' method and '+' operator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. (1) is similar to JMESPath. I remember using it in the AWS command line client.
The syntax of JMESPath is similar to jq
, e.g.
aws lambda list-functions \
--output text \
--query "Functions[].[FunctionName]"
Maybe something there could be useful for ScriptEngine's implementation too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main motivation for the dotted keys is that working with the context in SE YAML scripts often ends up being quite inconvenient. We often need to set a single parameter a couple of layers down the hierarchy and that means either many lines in the YAML scripts to express the nested access or (artificially) switching to inline dict syntax. The second point was that some SE tasks could not set nested context parameters, which is a stupid limitation. And internally, I was never happy with the strange Context
vs ContextUpdate
class construction, even less so because it is an implementation detail that was part of the API facing task developers.
I was searching for quite a while to find a package that would deliver dotted dicts. dotty_dict was a good candidate. However, I didn't need/want the indexing-with-integers feature and it turned out to be a bit complex given the extensions I wanted to make. But who knows, maybe in the future. It should be possible now to keep the Context
API for task developers while changing the underlying implementation.
I did not know about JMESPath, thanks for pointing out!
def test_include_not_found_raises_error(tmp_path): | ||
t = from_yaml( | ||
""" | ||
base.include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was curious if there would be tests for multiple dots, or .
encoded different ways (hex code, HTML encoded, etc.). Not sure if useful to test, but that's something I'd be curious to know if works or not 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this comment end up in a random place? Or am I missing the link to this particular lines?
Anyway, I try to answer: Multiple dots are allowed, what you get is the empty string as a (nested) key. I did actually think about it and did not come up with a good reason to disallow it. It is not explicitly tested for (in the pytests), just because it is not considered a special case. Do you have any good arguments for/against?
I have not really thought about different codings in keys (be it the separator or other parts of the key). Being into numerical computing for too long, I guess 😁
So it works/works not in as far as string.split
and string.partition
will handle it. If there is a good motivation and/or hints, we could follow up on this.
Thanks a lot for the feedback! It is appreciated! |
Note: There is still one checks failing ( |
Particularly, the first point allows for things like
instead of
and also to use
foo.bar
in Jinja expressions.