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

refactor: use the new flow/flownode links from the bpmn semantic in the predictions demo #515

Conversation

@csouchet csouchet added depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first refactoring Code refactoring labels Jun 21, 2023
@csouchet csouchet force-pushed the 476-Use_the_new_flow/flownode_links_from_the_BPMN_Semantic_in_the__Predictions__demo branch 8 times, most recently from 7520111 to f973d9a Compare June 22, 2023 09:51
@csouchet csouchet marked this pull request as ready for review June 22, 2023 09:54
@csouchet csouchet requested a review from tbouffard June 22, 2023 09:54
@tbouffard
Copy link
Member

I cannot review this PR until #514 is merged. There are commits coming from the dependent PRs and new commits in this one.
This make the review hard to do. So I will wait for the merge of the other PRs.

@csouchet csouchet force-pushed the 476-Use_the_new_flow/flownode_links_from_the_BPMN_Semantic_in_the__Predictions__demo branch from f973d9a to 3884708 Compare June 23, 2023 07:52
@csouchet csouchet removed the depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first label Jun 23, 2023
@@ -0,0 +1,43 @@
// From Bonita Day 2023 Demo
class PathResolver {
Copy link
Member

@tbouffard tbouffard Jun 27, 2023

Choose a reason for hiding this comment

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

issue: we cannot duplicate the code from the demo.
We already know it requires refactoring at least for process-analytics/bonita-day-demo-2023#52. There are also a lot of extra validation of the incoming/outgoing (verification that a related edge id exists in the model) whereas this should be guarantee by bpmn-visualization.
In addition, we plan to extract PathResolver from the bonita day demo process-analytics/bonita-day-demo-2023#36. If we duplicate the implementation here, we must track it and plan to use the shared code here. And this may not possible out of the box depending on the way we package the shared code. In particular, we are not sure to to provide an IIFE bundle for the shared lib.

Copy link
Member Author

Choose a reason for hiding this comment

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

To replace in another PR. See #260

Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

Let's talk about alternatives to the code duplicated from the bonita day demo (PathResolver).
As I mentioned in #514 (review), it's becoming increasingly clear to me that we should consider moving this demo to its own repository to benefit from the package manager, the shared library and the framework.

Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

After discussions with @csouchet, we agreed to merge this PR as it is today.
#260 includes a task to use the shared code of PathResolver when available.
We also agreed to review the extraction of this demo in a dedicated repository later.

@csouchet csouchet merged commit a9ffa39 into master Jul 10, 2023
@csouchet csouchet deleted the 476-Use_the_new_flow/flownode_links_from_the_BPMN_Semantic_in_the__Predictions__demo branch July 10, 2023 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REFACTOR] Use the new flow/flownode links from the BPMN Semantic in the "Predictions" demo
2 participants