Skip rules no-disconnect no-implicit-start no-implicit-end within AdHoc SubProcesses

[Buckwich]

Closes #180

Proposed Changes

Skip rules no-disconnect no-implicit-start no-implicit-end for activities within AdHoc SubProcesses

rule no-implicit-start no-implicit-end

Valid

AdHoc SubProcess still needs start & end

rule no-disconnect

Valid

Checklist

To ensure you provided everything we need to look at your PR:

• Brief textual description of the changes present
• Visual demo attached
• Steps to try out present, i.e. using the @bpmn-io/sr tool
• Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

[nikku]

Thanks for this PR, and the comprehensive visual description of what it does.

One thing that both of you discussed is whether it should only "disable for activities" or any flow-nodes, which is this case you attached, right?

Ad-hoc sub-processes are special, because start events and disconnected gateways are forbidden anyway, and elements are fine ("meant to be") disconnected there. Using bpmn-js-bpmnlint I validated this new rule against the recommended bpmnlint settings, some feedback:

To my knowledge, implicit start does not exist in the context of an ad-hoc sub-process. Elements must always be explicity activated.

Implicit ends do not exist inside an ad-hoc sub-process. It is completed by the user once "all work is done", whatever that means.

no-disconnected also generally does not make sense, within that element. We do have ad-hoc-sub-process as a rule that validates strictly connection semantics within an ad-hoc sub-process.

In this light, I propose to simplify your solution:

• The no-disconnected, no-implicit-end and no-implicit-start should be disabled for all flow nodes within an ad-hoc sub process

[nikku]

We also have to be careful to return false from a rule evaluation, because it will prevent entering the sub-tree to check nested nodes.

[barmac]

implicit start does not exist in the context of an ad-hoc sub-process
Implicit ends do not exist inside an ad-hoc sub-process

I agree with these statements.

no-disconnected also generally does not make sense, within that element. We do have ad-hoc-sub-process as a rule that validates strictly connection semantics within an ad-hoc sub-process.

Yes, but the rule does not apply to the gateways. Perhaps most suitable solution would be to disable no-disconnected for ad-hoc subprocess, and implement gateway validation in ad-hoc-sub-process (both incoming and outgoing required).

[nikku]

Perhaps most suitable solution would be to disable no-disconnected for ad-hoc subprocess, and implement gateway validation in ad-hoc-sub-process (both incoming and outgoing required).

Yes, I'd disable that rule. No, I'd not add dedicated validation for gateways, unless a user reports that as an issue. In other words, the following is an absolutely fine use of an ad-hoc sub-process to me. You can decide whether you want that "complex decision" to be taken, or any other decision, at any point in time:

This is also backed by the spec text you shared, I think.

---

Update: The following shows what things can still be an issue, but I think that these are edge cases. Especially the event-sub-process could be handled explicitly (independent of ad-hoc parent or not), because without following catch events it does not resemble a meaningful BPMN construct. The other one is a no-op:

[barmac]

Sure we can always wait for more feedback. Still, the main problem to solve is that activities are marked as disconnected/implicit-start/implicit-end where they should not be marked (false positive).

[Buckwich]

I've adjusted the rules to always skip if in an adhoc-subprocess. I've kept returning false. And removed the invalid test case as they are now valid. I'll probably also squash all commits into one as it is always the same line of codes

[barmac]

Suggestion: The PR title could be human readable. We will not be able to find "180-adhoc-activities" in the future.

[nikku]

Simple enough, thanks for following up! ⭐

Let's squash and merge things.