search

bpmn-io / bpmnlint

Validate BPMN diagrams based on configurable lint rules.
MIT License
121 stars 36 forks source link

Log bpmndi error to root level #61

Closed MaxTru closed 3 years ago

MaxTru commented 3 years ago

fixes upstream bpmn-io/bpmn-js-bpmnlint#27

nikku commented 3 years ago

Alternative would be to bake this logic into bpmn-js-bpmnlint as we've previously discussed. I.e. the visual integration could consider the fact that the element is not shown and bubble up the notification, displaying on the first visible element up the chain.

If we go for the strategy you propose in this PR we'd need to adopt it in all places, i.e. log errors with implementation details not on the actual detail but the (visible) element. The linter itself, from my point of view, should not be concerned about "what is the visible" element, but it should be the visual glue (bpmn-js-bpmnlint) to account for that.

Any specific reason you decided to go this route?

MaxTru commented 3 years ago

Alternative would be to bake this logic into bpmn-js-bpmnlint as we've previously discussed. I.e. the visual integration could consider the fact that the element is not shown and bubble up the notification, displaying on the first visible element up the chain.

If we go for the strategy you propose in this PR we'd need to adopt it in all places, i.e. log errors with implementation details not on the actual detail but the (visible) element. The linter itself, from my point of view, should not be concerned about "what is the visible" element, but it should be the visual glue (bpmn-js-bpmnlint) to account for that.

Any specific reason you decided to go this route?

Yeah true.

Working on bpmn-js-bpmnlint I realized that implementing the logic "if error is not displayable because of missing DI" is business logic, which should also in an ideal world not be the concern of bpmn-js-bpmnlint.

However, I forgot that using bpmnlint, one would like to be sure to always get the id of the acual affected element. This PR here would break this logic.

So trading off these two challenges, I agree and will go with changing directly in bpmn-js-bpmnlint.

Thanks!