71 from poc - dynamic import loading #225
Conversation
Bumps [feel-engine](https://github.com/camunda/feel-scala) from 1.16.0 to 1.16.1. - [Release notes](https://github.com/camunda/feel-scala/releases) - [Commits](camunda/feel-scala@1.16.0...1.16.1) --- updated-dependencies: - dependency-name: org.camunda.feel:feel-engine dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
…nda.feel-feel-engine-1.16.1 chore(deps): bump feel-engine from 1.16.0 to 1.16.1
|
@tracy-hires awesome. Thank you for contributing. 🎉 Currently, I'm very busy shipping important features in the FEEL engine. I don't have time to look at your PR immediately. I'll try to find some time in the next weeks. 🤞 |
|
🚋 I'll review the PR next week. |
There was a problem hiding this comment.
@tracy-hires I checked your changes closely. And, I like them, especially the introduction of a reference type. 👍
I see a few points that could be improved. See my comments for details.
I'll merge your PR as it is and add my own changes on top. This way, we can shorten the review cycle and move faster. 🚀
Thank you again for your contribution and your patience. 🎉
| override def resolve(): ParsedDecision = repository.getDecision(namespace, id) match { | ||
| case Right(found) => found | ||
| case Left(failure) => ParsedDecisionFailure(id, namespace, ExpressionFailure(failure.message)).resolve() | ||
| } |
There was a problem hiding this comment.
❌ The parsed objects (i.e. DTOs) should not contain the logic to load an imported decision/BKM.
Instead, we should keep the DTOs clean and simple, and move the loading into the evaluation logic.
| .map(bkm => { | ||
| ParsedInvocation(bindings, bkm) | ||
| .map(bkmRef => { | ||
| ParsedInvocation(bindings, bkmRef.resolve()) |
There was a problem hiding this comment.
❌ The invocation doesn't work for imported BKMs.
If the invocation requires an imported BKM and the BKM is not parsed yet, then the resolve() will throw an exception.
Instead of resolving the BKM during the parsing, we should load the BKM during the evaluation. The same way as we do for required decisions/BKMs.
Description
This addresses the following todos referenced in #222:
Related issues
related to #71
closes #