expose Eval function for custom partial eval#94
expose Eval function for custom partial eval#94philhassey merged 3 commits intocedar-policy:mainfrom
Conversation
philhassey
left a comment
philhassey
left a comment
There was a problem hiding this comment.
Looks good, just an ask for documentation. Also, you'll have to appease the DCO requirement. Here's a doc about it, I think it explains the gist:
x/exp/eval/eval.go
Outdated
| "github.com/cedar-policy/cedar-go/x/exp/ast" | ||
| ) | ||
|
|
||
| func Eval(n ast.IsNode, env eval.Env) (types.Value, error) { |
There was a problem hiding this comment.
Add a one sentence piece of documentation that explains what this function does.
There was a problem hiding this comment.
Also a couple unit tests just to prove it works are needed. Since the internal implementation has all the exhaustive unit tests, I think even just a few would be nice. Maybe one that returns an error and one that does not.
Also, it looks like it does require an eval.Env which isn't publicly exposed, so you may need to expose that as well in this package. It'd be fine to have a type Eval = eval.Env and put a doc string above it.
There was a problem hiding this comment.
ok, will fix these problems
Signed-off-by: jaredzhou <zhouzhenyu1313@163.com>
Signed-off-by: jaredzhou <zhouzhenyu1313@163.com>
Addresses cedar-policy#94 Signed-off-by: philhassey <phil@strongdm.com>
|
Some linter has been updated and is now tripping on something. I've made a PR to address that and once it is merged I think this PR will pass the tests. |
Addresses cedar-policy#94 Signed-off-by: philhassey <phil@strongdm.com>
|
If you merge from |
Signed-off-by: jaredzhou <zhouzhenyu1313@163.com>
|
thank you！ |
2 participants
Issue #, if available:
#91
Description of changes: