-
Notifications
You must be signed in to change notification settings - Fork 121
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
feat: ops eval hook #1450
base: main
Are you sure you want to change the base?
feat: ops eval hook #1450
Conversation
"""Eval an expression in the context of the charm and print the result to stdout.""" | ||
import json | ||
|
||
globs = {'self': self.charm, 'ops': ops, 'json': json} |
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 feel a slippery slope here: start with self, add ops, then json.dumps()
because surely it's useful, then maybe yaml? pathlib? os? stdlib 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.
Yeah, I agree. self
seems fine as you kinda need the charm instance, ops
seems reasonable, but anything beyond that I'd see if we can find a different way to import stuff. If we go the "exec" route (rather than "eval"), I guess you can do import json; json.dumps(x)
.
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.
to be fair, jhack also has a sister command, jhack script
, that allows loading in a python file with a main(charm:Charm)
entrypoint for the kind of 'more complex' scenarios where you may need to import stuff and the like
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.
eval is mostly for inspection, not so much mutation. json felt like a universal, low-barrier addition. Yaml I wouldn't object to, but pathlib, os, operator feel too much. It's not about allowing you to golf complex operations into a string, it's about inspecting live data
@@ -20,6 +20,7 @@ | |||
import subprocess | |||
import sys | |||
import warnings | |||
from optparse import Option |
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.
OT: unused, afaics
Proposal to address #1449