Skip to content
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: add implementation rule #11

Merged
merged 2 commits into from
Apr 21, 2021
Merged

feat: add implementation rule #11

merged 2 commits into from
Apr 21, 2021

Conversation

barmac
Copy link
Contributor

@barmac barmac commented Apr 19, 2021

Closes #4

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Apr 19, 2021
@barmac
Copy link
Contributor Author

barmac commented Apr 19, 2021

On the other hand, the implementation details are not rendered on a diagram, so it doesn't seem to make sense to use pictures to visualise the rule. Still, there is no option in the tool to opt-out from generating images for selected diagrams.

The documentation does not include an image due to the fact that difference is not rendered on the diagram. This is also the reason why I resigned from the diagram examples and used code snippets instead.

@barmac barmac marked this pull request as ready for review April 19, 2021 10:29
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Apr 19, 2021
@barmac barmac requested review from nikku and MaxTru April 19, 2021 10:30
rules/implementation.js Outdated Show resolved Hide resolved
rules/implementation.js Outdated Show resolved Hide resolved
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small comments.

@barmac barmac force-pushed the 4-implementation branch 3 times, most recently from d00c99b to 1ef5f9a Compare April 19, 2021 11:32
@barmac barmac requested a review from nikku April 19, 2021 11:32
package.json Show resolved Hide resolved
@barmac
Copy link
Contributor Author

barmac commented Apr 20, 2021

Got feedback from @ThorbenLindhauer that we should disallow missing implementation for the Message Throw/End Events. So this would be a way to promote best practices.

From readme:

Camunda modeling guidelines, packed as a bpmnlint plug-in.

In that case, I think it makes sense to be more strict than the engine.

@nikku
Copy link
Member

nikku commented Apr 20, 2021

Let's implement the strict message event definition validation (#11 (comment)) and we're good I think!

package.json Outdated Show resolved Hide resolved
@nikku nikku self-requested a review April 21, 2021 07:24
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you removed most of the rough edges.

One thing I thought about: How much sense does it make to activate this rule in non-executable processes?

In BPMN we distinguish between these. Non executable processes are meant for documentation only and not picked up by the engine. I'd assume I can do anything in there, with no such limit.

@barmac
Copy link
Contributor Author

barmac commented Apr 21, 2021

Yeah, that makes sense. I haven't thought about it. I'll extend the test case to not bug the user about non-executable processes.

@barmac
Copy link
Contributor Author

barmac commented Apr 21, 2021

Now we ignore non-executable processes.

Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 🍰 🎉

@merge-me merge-me bot merged commit 714b0e8 into master Apr 21, 2021
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Apr 21, 2021
@nikku nikku deleted the 4-implementation branch April 21, 2021 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate presence of implementation on camunda:ServiceTaskLike
2 participants