Conversation
2bb6c52 to
95f36af
Compare
barmac
left a comment
There was a problem hiding this comment.
PR needs a cleanup, cf. the comments.
I'd suggest to squash the commits from bpmn-io/feel-editor as we won't benefit from them in this project.
7414a77 to
2a9ad9a
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR migrates Camunda FEEL builtins from the feel-editor repository to a separate, dedicated repository. The migration establishes a standalone package that can be consumed by various tools that need Camunda FEEL extensions.
- Introduces build automation to extract builtins from Camunda documentation
- Creates a comprehensive test suite for the parsing and compilation functionality
- Sets up package infrastructure with proper exports, CI/CD, and documentation
Reviewed Changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Configures the new package with build scripts, dependencies, and module exports |
| lib/index.js | Main entry point exporting camundaBuiltins |
| lib/camundaBuiltins.js | Generated file containing 1700+ lines of compiled builtin functions |
| tasks/compileBuiltins.js | Build script that processes Markdown docs and generates the builtins file |
| tasks/utils/parseBuiltins.js | Utility functions for parsing function signatures from documentation |
| test/spec/lib/camundaBuiltins.spec.js | Integration tests verifying the exported builtins structure |
| test/spec/tasks/utils/parseBuiltins.spec.js | Unit tests for the parsing utility functions |
Comments suppressed due to low confidence (1)
test/spec/lib/camundaBuiltins.spec.js:5
- The describe block name 'lib/compileBuiltins' is inconsistent with what is being tested. This should be 'lib/camundaBuiltins' to match the actual module being tested.
describe('lib/compileBuiltins', function() {
nikku
left a comment
There was a problem hiding this comment.
@Buckwich as I understood the intend of this PR is to make this library ready to release.
Find attached some updates that I'd like us to merge/incorporate if possible:
- drop
boost(I don't know why this should be in scope of this library), it is not documented (a651c43) - ensure we test the types we ship (081483c), but also as we're on it, validate the code base :)
- ensure the eslint configuration validates our code base (9ae2d92)
- A CI task that ensures we recognize when new built-ins arrive - ce77184
- Improved test coverage of the actual "thing" (b2d400f) and dropping some legacy that is no longer needed (b10df10)
The code base is now linted, and type checked, and should be good to release. Please have a look at my additions to the PR.
|
Consider what you want to incorporate; I'm happy to spin off, i.e. the CI changes to a separate PR. |
|
I've just realized you worked on "automatic update" of the built-ins via #2. I'd be interested how you evaluate automatic update vs. "knowing new built-ins are there". |
|
FYI: I'll wait to merge this PR until I also have the auto update CI done. I'll use the chinese comma fix to test that, but want that fix included in the first release |
Related to camunda/camunda-modeler#3983 Co-authored-by: Nico Rehwaldt <nico.rehwaldt@camunda.com>
|
Tested #3 for comments in PRs. Will go ahead and release now (change: https://github.com/camunda/feel-builtins/compare/77c716f1b397f498e78179c44d63fc2fdbe1a416..2057d2bcc6b690b4ac13093e93352752a08de612) |
Related to camunda/camunda-modeler#3983
Proposed Changes
Migration of camundaBuiltins into separate repo.
As this PR is just exporting a list of variables, nothing much to try out (or even test) here. But you can checkout camunda/bpmnlint-plugin-camunda-compat#206 to see how it integrates
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/srtoolCloses {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}