[Workflow] Add Mermaid.js dumper #40171
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
There is a single dumper output here, while other dumpers have different output for the state machine vs workflow cases, because the behavior of transitions is different. In full-fledged workflows, each transition can have multiple |
| } | ||
|
|
||
| $placeNodeFormat = '%s'.$labelShape; | ||
| $placeNode = sprintf($placeNodeFormat, $placeName, $placeLabel); |
There was a problem hiding this comment.
you need to apply proper escaping on labels, to ensure the generated graph is valid, because labels can be anything: https://mermaid-js.github.io/mermaid/#/flowchart?id=special-characters-that-break-syntax
There was a problem hiding this comment.
also, be careful than the lowercase end identifier is reserved and cannot be used as a node identifier.
|
@stof There is an explicit diagram syntax for state machines in Mermaid. However, that does not support styling the same way the flowchart syntax does. I can see two ways forward here: Either go the same route as the PlantUML Dumper and just use a different line style for state machines or choose the Graphviz strategy of having another class handling the specifics of state machines. I currently favour the latter, am however unsure because of the styling restrictions. EDIT: I had to push my current state, but it is not ready for review yet (in my opinion), just may have to switch machines. |
|
@eFrane actually, I would say that the work you did at the time of my previous review was actually more appropriate for the dumping of state machines (look at the graphviz and PlantUML outputs for both cases). so I think the dumper should keep using Regarding 1 class vs 2 classes for the dumping, I would say that it depends how much code depends on the cases and how much is shared between them. |
1d9a488 to
0b830c0
Compare
|
@stof After having an embarrassingly long stare at the existing dumpers and the documentation, I finally understood your point about the differing dumping styles for workflows and state machines. As there was quite a bit of added code, there may be new non-symfony ways I did things. Apart from that, I also added support for markings. In the meantime, I will (finally) start on the documentation update PR for this. |
bae73e2 to
ada6f7d
Compare
|
Thanks @eFrane for working on this feature, this is much appreciated. |
This PR was merged into the 5.3-dev branch. Discussion ---------- [Workflow] Add info about MermaidDumper <!-- If your pull request fixes a BUG, use the oldest maintained branch that contains the bug (see https://symfony.com/releases for the list of maintained branches). If your pull request documents a NEW FEATURE, use the same Symfony branch where the feature was introduced (and `5.x` for features of unreleased versions). --> This updates the documentation for symfony/symfony#40171. Commits ------- b7340c7 [Workflow] Add info about MermaidDumper
Mermaid is - next to PlantUML - one of the most popular simple graphing solutions. This workflow dumper mirrors the feature set of the PlantUML dumper except that Mermaid does not currently support colored transitions.
Things I need help with:
I basically tried to copy the code style of the surrounding files and hope everything is conforming. Please let me know if I missed something.I see, that's the magic of fabbot. Nice. ❤️