-
Notifications
You must be signed in to change notification settings - Fork 4k
Add vega-interpreter to Vega Lite Charts #5462
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
Conversation
c4c696b to
f934b91
Compare
|
Not entirely sure how I ought to write tests for this... |
Yep, that sounds pretty complicated. This might require some research, but I think we cannot test this through our unit tests (jest, pytest) since there isn't any browser involved. So, we either have to test it via e2e tests by adding it somehow to the One super hacky way I can think of would be the following: inject the header into the Another cleaner solution might be that we add an optional config option (e.g. |
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.
nit: Do you know if this is actually required? I think it is used as default if ast is set to true:
https://github.com/vega/vega-embed/blob/88fb1224cbb2e51644849a20f1e7ba629629bab9/src/embed.ts#L378
But there is most likely no harm in explicitly specifying this.
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.
Agreed it looks like its passed as the default, just thought it would be more straightforward to understand if it was explicitly passed to expr:
lukasmasuch
left a comment
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.
Overall looks good 👍 We somehow need to figure out if there is a good way to test this. Maybe we can bring this up as a discussion topic in the next standup to discuss some of the options.
2f484eb to
56c1623
Compare
kmcgrady
left a comment
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.
Vega is BSD 3-Clause so I approve this dependency change
This reverts commit 9e9c4bc.
)"" This reverts commit 7a82569.
This reverts commit 9e9c4bc.
)"" This reverts commit 1c3bc7e.
This reverts commit 9e9c4bc.
)"" This reverts commit 4356302.
📚 Context
Integrating the expression interpreter with
vega-embedto avoidunsafe-evalpermission for CSP🧠 Description of Changes
ast: trueand expression interpreter tovega-embed🧪 Testing Done
🌐 References
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.