-
Notifications
You must be signed in to change notification settings - Fork 2
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: support execution listeners #168
Conversation
5513a78
to
29b42cb
Compare
29b42cb
to
3841463
Compare
module.exports = skipInNonExecutableProcess(function() { | ||
function check(node, reporter) { | ||
|
||
console.log(node); |
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.
We probably don't want that.
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.
That's right 😅
return; | ||
} | ||
|
||
const errors = hasDuplicatedExecutionListeners(executionListeners, node); |
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.
Have we considered using the existing helper used by a similar rule (https://github.com/camunda/bpmnlint-plugin-camunda-compat/blob/main/rules/camunda-cloud/duplicate-task-headers.js#L28)?
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 guess it's because duplication requires two duplicate properties instead of just one. We could still adjust the helper to allow multiple properties.
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 have considered this and decided against it due to the adjustment needed. If we have more cases like this, we may abstract it away, but right now I'd consider it YAGNI.
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'm not a huge fan of using ERROR_TYPES.PROPERTY_VALUE_DUPLICATED
and then duplicatedProperty: 'type'
since it doesn't mention the event type at all. We should probably have a separate type PROPERTY_VALUES_DUPLICATED
and include all the data necessary to create a custom error message.
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'd just add a hasDuplicatedPropertiesValues
helper that encapsulates the logic so the rule simply calls hasDuplicatedPropertiesValues(taskHeaders, 'listeners', [ 'eventType', 'type' ], node)
. 🤔
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'll try that.
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.
@philippfromme please have a look now.
Related to camunda/camunda-modeler#3951 feat: add `no-duplicate-execution-listeners` rule feat: add `no-execution-listeners` rule feat: add `execution-listener` rule
897e806
to
1c0aa1f
Compare
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.
Great job!
Related to camunda/camunda-modeler#3951
Contains:
no-duplicate-execution-listeners
rule - to prevent duplicates rejected by the engineno-execution-listeners
rule - to prevent usage with unsupported engine versionexecution-listener
rule - to ensure correct configuration