Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Workflow] Add PlantUML dumper to workflow:dump command#24705
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Plopix commentedOct 27, 2017
That is failing on Windows, but I don't really know why, probably a weird Windows specificity, or the END OF LINE? Thanks! |
| CHANGELOG | ||
| ========= | ||
| 3.5.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It will be4.1.0 not3.5.0 (which never will be released)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok
| } | ||
| return | ||
| '@startuml'.PHP_EOL.static::SYMFONY_LOGO.PHP_EOL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be in previous line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok
| constMARKED ='marked'; | ||
| constSYMFONY_LOGO ='sprite $sf_logo [81x20/16z] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Better & probably safer than string comparison would be to save them as fixtures & compare results with pregenerated fixture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am not sure I understand here what you are proposing. Can you tell me more and I will be happy to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think I got it and I made changes accordingly.
Plopix commentedOct 28, 2017
@Nyholm or@lyrixx |
nicolas-grekas commentedOct 28, 2017
DST time, transient failure will be fixed tomorrow hopefully :) |
lyrixx commentedNov 7, 2017
Oups Sorry I forgot to reply. Yes it's wanted. As a state machine can have many transition with the same name, the graph could be very ugly. that's why we did this. |
| thrownew \InvalidArgumentException(sprintf('No service found for "workflow.%1$s" nor "state_machine.%1$s".',$serviceId)); | ||
| } | ||
| /** @var \Symfony\Component\Workflow\Workflow $workflow */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks like it's useless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not for PHPStorm ;) but I can remove it
Plopix commentedNov 8, 2017
@lyrixx I have added an option to let the user decide which transition he wants. |
lyrixx commentedNov 8, 2017
Thanks. I will try your PR as soon as possible ;) |
nicolas-grekas commentedDec 8, 2017
@lyrixx time to review? :) |
| publicfunction__construct($transitionType) | ||
| { | ||
| $this->transitionType =static::DEFAULT_TRANSITION_TYPE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You don't need to do this since it's already defaulted when you do$input->getOption('transition')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree but this class could be used differently and without the command then I think it is correct to initialize it. no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not using
protected$transitionType =self::DEFAULT_TRANSITION_TYPE;
instead ?
I may be missing something but it feels better to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No that is me, I don't know I wanted to avoid that because of php versions but now for 4.x it is php 7 then we are all good to do that. And I think we were good before too...I was wrong. It is better now ;)
| publicfunction__construct($transitionType) | ||
| { | ||
| $this->transitionType =static::DEFAULT_TRANSITION_TYPE; | ||
| if ($transitionType ==static::SQUARE_TRANSITION_TYPE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if (static::SQUARE_TRANSITION_TYPE ===$transitionType)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same as above comment
| protectedfunctionisSquareTransitionType() | ||
| { | ||
| return$this->transitionType ==static::SQUARE_TRANSITION_TYPE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why not=== ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok will change
| $code[] ="agent{$transition->getName()}"; | ||
| } | ||
| } | ||
| foreach ($definition->getTransitions()as$transition) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It feels like alot of complexity comparing to the GraphvizDumper, isn't in the class something you could re-use to get arrays and then transform them directly ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not really sure I get what you want me to do. I agree it is not really straightforward but it is an efficient way to manage both TransitionType, GraphvizDumper as 2 classes to do that. I did not see the value for the PlantUML one, and I still don't see it. (but I would love to have your opinion)
I could do that in another method and then merge arrays... is it what you would like?.
Let me know!
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was just thinking on how much this adds complexity. I guess if merging the arrays cost more in performance than what you are doing here let's keep this. (I think it will be anyway).
I just wanted to know more about this ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok
Plopix commentedDec 12, 2017
I have updated the PR and rebased. Not sure why the tests are failing I think it is not related to this code, |
| publicfunction__construct($transitionType =null) | ||
| { | ||
| if ($transitionType ===static::SQUARE_TRANSITION_TYPE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should use yoda style, btw I don't know if it's really needed to do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- yoda style done.
- it is a way to kind of enforcing the 2 types and those 2 type only the default one or
square
It would work without, but the value oftransitionTypecould be inconsistent. Let me know your thoughts
| protectedfunctionisSquareTransitionType() | ||
| { | ||
| return$this->transitionType ===static::SQUARE_TRANSITION_TYPE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yoda style please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
| if (isset($options['name'])) { | ||
| $code[] ="title{$options['name']}"; | ||
| } | ||
| if (isset($options['skinparams']) &&count($options['skinparams']) >0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think you need to count here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
correct I replaced withis_array to ensure theforeach won't fail if someone send something else
Simperfit left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Some minor comments, but great improvment thanks !
Plopix commentedDec 12, 2017
updated@Simperfit (we are almost there ;) ) |
lyrixx left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I let few comments on the Command Class. It seems you did not test it on real
project :/
And I followed you doc and it does not work. the program open a dialog windows
where I'm supposed to select a folder. I'm a bit lost.
Finally, I'm not sure if plantuml is really mainstream. By default the website
is in French :/http://plantuml.com/
So for now I'm not sure we want this support in the Core. What do others think?
| useSymfony\Component\Workflow\Dumper\GraphvizDumper; | ||
| useSymfony\Component\Workflow\Dumper\StateMachineGraphvizDumper; | ||
| useSymfony\Component\Workflow\Marking; | ||
| useSymfony\Component\Console\Input\InputOption; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Input Option Is already imported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed.
| useSymfony\Component\Workflow\Dumper\StateMachineGraphvizDumper; | ||
| useSymfony\Component\Workflow\Marking; | ||
| useSymfony\Component\Console\Input\InputOption; | ||
| useSymfony\Component\Workflow\Dumper\PlantUmlDumper; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you sort alluses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok
| newInputArgument('marking', InputArgument::IS_ARRAY,'A marking (a list of places)'), | ||
| newInputOption('label','l', InputArgument::OPTIONAL,'Labels a graph'), | ||
| newInputOption('format',null, InputArgument::OPTIONAL,'The dump format','dot'), | ||
| newInputOption( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We don't do that in Symfony. All arguments should be on one line..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
alright, that is fixed.
| newInputOption('label','l', InputArgument::OPTIONAL,'Labels a graph'), | ||
| newInputOption('format',null, InputArgument::OPTIONAL,'The dump format','dot'), | ||
| newInputOption( | ||
| 'transition', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
puml_transition_format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed.
| newInputOption( | ||
| 'transition', | ||
| null, | ||
| InputArgument::OPTIONAL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is wrong:
- It's an option, not an arguement
- The value is required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
correct, fixed.
| %command.full_name% <workflow name> | dot -Tpng > workflow.png | ||
| <info>DOT</info>: %command.full_name% <workflow name> | dot -Tpng > workflow.png | ||
| <info>PUML</info>: %command.full_name% <workflow name> --format=puml -p | java -jar plantuml.jar > workflow.png |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is-p ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That is a typo (not present in the PR doc) the-p, as mentioned in the PR doc is forpiping. But it is for plantuml.jar... not for the command. I have fixed that too.
| newInputArgument('name', InputArgument::REQUIRED,'A workflow name'), | ||
| newInputArgument('marking', InputArgument::IS_ARRAY,'A marking (a list of places)'), | ||
| newInputOption('label','l', InputArgument::OPTIONAL,'Labels a graph'), | ||
| newInputOption('format',null, InputArgument::OPTIONAL,'The dump format','dot'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"The dump format [dot|puml]"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
format is a bad name because it's already used in the --help command.
Sobin/console workflow:dump article --format=puml -h do not work anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
good catch I have renamed bydump_format
| if ('puml' ===$format) { | ||
| $dumper =newPlantUmlDumper($transitionType); | ||
| }else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
use an else if here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok done.
| /** | ||
| * PlantUmlDumper dumps a workflow as a PlantUML file. | ||
| * | ||
| * You can convert the generated dot file with the plantuml.jar utility (http://plantuml.com/): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suppose it does not generate a dot file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
that is correct, I have pushed the correction. thx!
| knh4RrvOKzxZfLV3s0rs_R_1SdYt3VxeQ1_y2_W2 | ||
| }'; | ||
| constINITIAL ='initial'; | ||
| constMARKED ='marked'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn't some of these be private constants ?
| privatefunctionisSquareTransitionType():bool | ||
| { | ||
| returnstatic::SQUARE_TRANSITION_TYPE ===$this->transitionType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
useself:: to access constant. There is no reason to make them an extension point by allowing changing their values (and the code would not work anyway in such case, due to other places)
| return$start; | ||
| } | ||
| return$start.static::SYMFONY_LOGO.PHP_EOL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
self: here as well
| return$end; | ||
| } | ||
| returnPHP_EOL.'footer \nGenerated by <$sf_logo> **Workflow Component** and **PlantUML**'.$end; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are you sure about the\n here, which is not a newline char ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes I think so, the\n is working as expected.
stof commentedJan 23, 2018
The dump of the workflow in#24705 (comment) looks confusing to me. It looks like there are 2 independant |
lyrixx commentedJan 23, 2018
Oh@stof Is totally right. This PR could not be merged in this state. |
stof commentedJan 23, 2018
The current dumper is fine for the case of state machines though |
Plopix commentedJan 23, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@stof thank you I have updated the code. From my point of view, I don't find it that confusing as I know that I can have only one You said:
But here:https://symfony.com/doc/current/workflow/state-machines.html The PR is aiming to let the choice to the user to dump it with On the cases provided in the doc and on the Pull Request:https://symfony.com/doc/current/workflow/state-machines.html Blog Publishing:https://symfony.com/doc/current/workflow/usage.html Article:https://github.com/lyrixx/SFLive-Paris2016-Workflow/blob/master/config/packages/workflow.yaml#L3 I think that is working, I am not sure what you are expecting, I see at least 3 options in which we could go:
Let me know |
lyrixx commentedJan 24, 2018
It's definitively the way to go ;) |
Plopix commentedJan 25, 2018
hey@lyrixx@stof I have done the option 1/ For the sake of my understanding, I would love if you could you explain why "arrows" are not valid sometimes? In this example, for instance, it feels simpler to have arrows. But I would understand if the reason is to force a kind of consistency for each type. Let me know! |
lyrixx commentedJan 26, 2018
As you can see on the previous schema, If you don't draw the transition (square), you haveno way to know how many transition there are. And of course the 2 workflows (line 1 and line 2) are totally different. |
Plopix commentedJan 26, 2018 • edited by lyrixx
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by lyrixx
Uh oh!
There was an error while loading.Please reload this page.
ok makes sense ;-) all good to merge then maybe ;-) |
fabpot commentedFeb 7, 2018
Thank you@Plopix. |
…d (Plopix)This PR was merged into the 4.1-dev branch.Discussion----------[Workflow] Add PlantUML dumper to workflow:dump command| Q | A| ------------- | ---| Branch | 4.1| Bug fix | no| New feature | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| License | MIT| Doc PR | Will do depending on the comments about that PRHello,`workflow:dump` exports workflows in `dot` format. To me, this format is not the easiest and not the simplest to use. Styles and logic are mixed together which makes it hard to read.[PlantUML](http://plantuml.com/) is a tool based on Graphviz like Dot but thatgenerates nicer diagrams, more readable and most of all easier to adapt to yourdocumentation. Just copy and paste the PUML on the website you will see the rendering live.Also, there is a [PHPStorm Plugin](https://plugins.jetbrains.com/plugin/7017-plantuml-integration) and [plenty of integration](http://plantuml.com/running) of this format.This PR adds 2 options* a `--dump-format=puml` option to the `workflow:dump` command to generate the workflows in PlantUML.* a `--puml-transition-format=square|arrow` option to the `workflow:dump` command to generate the workflows in PlantUML using a square shape or arrow for the transition. (see below)The conversion requires the PlantUML JAR, and can be used like that:```bashphp bin/console workflow:dump pull_request --dump-format=puml | java -jar plantuml.jar -p > workflow.png```> don't forget the `-p` to enable the "piping"Here is an example with `pull_request` workflow of the documentation (with no style and no marking):```@startumltitle pull_requeststate start <<initial>>state codingstate travisstate reviewstate mergedstate closedstart --> travis: submitcoding --> travis: updatetravis --> travis: updatereview --> travis: updatetravis --> review: wait_for_reviewreview --> coding: request_changereview --> merged: acceptreview --> closed: rejectclosed --> review: reopen@enduml```As PlantUML let us define styles, I have provided some by default that the user can override.Adding some marking:```bashphp bin/console workflow:dump pull_request travis review --dump-format=puml```will give us:```@startumlsprite $sf_logo [81x20/16z] {hPNRaYiX24K1xwBo_tyx6-qaCtDEJ-KXLYMTLbp0HWcHZr3KRDJ8z94HG3jZn4_mijbQ2ryJoFePtXLWA_qxyGy19DpdY_10z11ZAbGjFHRwcEbcKx5-wqsVyIMo8StMCHKh8ZUxnEwrZiwRAUOvy1lLcPQF4lEFAjhzMd5WOAqvKflS0Enx8PbihiSYXM8ClGVAseIWTAjCgVSAcnYbQG79xKFsZ0VnDCNc7AVBoPSMcTsXUnrujbYjjz0NnsObkTgnmolqJD4QgGUYTQiNe8eIjtx4b6Vv8nPGpncn3NJ8Geo9W9VW2wGACm_JzgIO8A8KXr2jUBCVGEAAJSZ6JUlsNnmOzmIYti9G7bjL8InaHM9G40NkwTG7OxrggvNIejA8AZuqyWjOzTIKi-wwYvjeHYesSWuPiTGDN5THzkYLU4MD5r2_0PDhG7LIUG33z5HtM6CP3icyWEVOS61sD_2ZsBfJdbVAqM53XHDUwhY0TAwPug3OG9NonRFhO8ynF3I4unuAMDHmSrXH57V1RGvl9jafuZF9ZhqjWOEh98y0tUYGsUxkBSllIyBdT2oM5Fn2-ut-fzsq_cQNuL6Uvwqrknh4RrvOKzxZfLV3s0rs_R_1SdYt3VxeQ1_y2_W2}title pull_requestskinparam titleBorderRoundCorner 15skinparam titleBorderThickness 2skinparam state { BackgroundColor<<initial>> #87b741 BackgroundColor<<marked>> #3887C6 BorderColor #3887C6 BorderColor<<marked>> Black FontColor<<marked>> White}state start <<initial>>state codingstate travis <<marked>>state review <<marked>>state mergedstate closedstart --> travis: submitcoding --> travis: updatetravis --> travis: updatereview --> travis: updatetravis --> review: wait_for_reviewreview --> coding: request_changereview --> merged: acceptreview --> closed: rejectclosed --> review: reopenfooter \nGenerated by <$sf_logo> **Workflow Component** and **PlantUML**@enduml```Which gives you that:With `square` as transition, it gives you that:Hope you will find that interesting!Commits-------1497d36 Add option to the workflow:dump command to allow PlantUML format dump
lyrixx commentedFeb 7, 2018
👏 Thanks for your hard work on this feature. I just tested it and it works very well. |
This PR was submitted for the 4.0 branch but it was merged into the master branch instead (closes#9220).Discussion----------[Workflow] PlantUML Dump documentationDocumentation related tosymfony/symfony#24705This PR aims to explain that the PlantUML dump format is available.Platform.sh URL:http://pr-9220-x7nzdnq-6qmocelev2lwe.eu.platform.sh/workflow/dumping-workflows.htmlCommits-------b62d961 [Workflow] PlantUML Dump documentation
Uh oh!
There was an error while loading.Please reload this page.
Hello,
workflow:dumpexports workflows indotformat. To me, this format is not the easiest and not the simplest to use. Styles and logic are mixed together which makes it hard to read.PlantUML is a tool based on Graphviz like Dot but that
generates nicer diagrams, more readable and most of all easier to adapt to your
documentation. Just copy and paste the PUML on the website you will see the rendering live.
Also, there is aPHPStorm Plugin andplenty of integration of this format.
This PR adds 2 options
--dump-format=pumloption to theworkflow:dumpcommand to generate the workflows in PlantUML.The conversion requires the PlantUML JAR, and can be used like that:
Here is an example with
pull_requestworkflow of the documentation (with no style and no marking):As PlantUML let us define styles, I have provided some by default that the user can override.
Adding some marking:
will give us:
Which gives you that:
With
squareas transition, it gives you that:Hope you will find that interesting!