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

Documented the workflow metadata [redux] #11209

Merged
merged 17 commits into from Apr 17, 2019

Conversation

@pbowyer
Copy link
Contributor

pbowyer commented Mar 23, 2019

With @javiereguiluz's permission I have taken #9476 and incorporated the comments and code examples into it.

This is the start of a push to improve the Workflow documentation. The focus now is on expanding the documentation by merging pull requests; organizing it will come later.

Show resolved Hide resolved workflow/usage.rst Outdated

@HeahDude HeahDude added this to the 4.2 milestone Mar 23, 2019

@HeahDude HeahDude added the Workflow label Mar 23, 2019

@HeahDude
Copy link
Member

HeahDude left a comment

This a great contribution, thanks a lot!

Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated

pbowyer added a commit to pbowyer/symfony-docs that referenced this pull request Mar 23, 2019

pbowyer added a commit to pbowyer/symfony-docs that referenced this pull request Mar 23, 2019

Add an introduction as suggested in symfony#11209 (comment)
When symfony#9465 is finished, link to details on Transition Blockers.
@HeahDude
Copy link
Member

HeahDude left a comment

I've left some minor comments, but that looks great. Thank you again for your work on this.

Note that we can do some minor changes when merging if you don't want to bother with specific formatting.

Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
@pbowyer

This comment has been minimized.

Copy link
Contributor Author

pbowyer commented Mar 23, 2019

@HeahDude re "specific formatting" I've noticed the file uses a mix of array( ... ) and [ ... ]. Which should be used?

When I took Javier's commits the branch ended up being created against symfony-docs/master. Can you make it be against the right branch when you merge?

@HeahDude

This comment has been minimized.

Copy link
Member

HeahDude commented Mar 23, 2019

I've noticed the file uses a mix of array( ... ) and [ ... ]. Which should be used?

Now we use the new short syntax, since the lowest supported version is 3.4 having PHP 5.5 as minimum.

When I took Javier's commits the branch ended up being created against symfony-docs/master. Can you make it be against the right branch when you merge?

4.1 was master back then, now we need to merge into 4.2 but we can do it on merge thanks to the CLI tool we use to maintain the docs.
Thanks!

Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst
@pbowyer

This comment has been minimized.

Copy link
Contributor Author

pbowyer commented Mar 27, 2019

Re the code style comment above which is outstanding: Is there a linter for code examples? An automated formatter?

I imagine we stick to Symfony's code style, but as Phpstorm's Symfony2 preset seems to differ in small ways from the current Symfony format, it would be good to have a definitive formatter to use.

@OskarStark
Copy link
Contributor

OskarStark left a comment

I left a few comments

Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
@lyrixx

lyrixx approved these changes Apr 7, 2019

Copy link
Member

lyrixx left a comment

Except one little comment big 👍

Thanks you a lot for your work on this PR

Show resolved Hide resolved workflow/usage.rst Outdated
@pbowyer

This comment has been minimized.

Copy link
Contributor Author

pbowyer commented Apr 7, 2019

@OskarStark Your code review is showing as "Changes requested" - are you satisfied with the changes that were applied?

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Apr 7, 2019

@pbowyer This PR conflicts. Could you try to rebase it and fix the conflict? Thanks

@pbowyer

This comment has been minimized.

Copy link
Contributor Author

pbowyer commented Apr 7, 2019

@lyrixx Of course. 4.2 is the correct one to rebase onto, I believe?

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Apr 7, 2019

Exactly 👍 You need to rebase on top of 4.2 and update the target branch on github
Thanks again

@pbowyer pbowyer force-pushed the pbowyer:pull/9476-workflow-metadata branch from e580f3c to 6080aa8 Apr 7, 2019

@pbowyer pbowyer changed the base branch from master to 4.2 Apr 7, 2019

@pbowyer

This comment has been minimized.

Copy link
Contributor Author

pbowyer commented Apr 7, 2019

@lyrixx There we go 😁

@lyrixx

lyrixx approved these changes Apr 7, 2019

@pbowyer

This comment has been minimized.

Copy link
Contributor Author

pbowyer commented Apr 7, 2019

There may be one newline missing:

As configured, the following property is used by the marking store::
.. code-block:: php

I think there should be a blank line between the two?

Show resolved Hide resolved workflow/usage.rst Outdated
Show resolved Hide resolved workflow/usage.rst Outdated
@OskarStark
Copy link
Contributor

OskarStark left a comment

Thank you for your patience ✌🏻

@lyrixx

lyrixx approved these changes Apr 8, 2019

return;
}

$explanation = $event->getMetadata('explanation', $event->getTransition());

This comment has been minimized.

Copy link
@noniagriconomie

noniagriconomie Apr 9, 2019

Contributor

@pbowyer I think you should add an explanation entry in the metadata of one publish transition so that a newcomer can directly spot the usage and the link between transition, place, etc

@noniagriconomie

This comment has been minimized.

Copy link
Contributor

noniagriconomie commented Apr 10, 2019

Ref to #9475

@pbowyer

This comment has been minimized.

Copy link
Contributor Author

pbowyer commented Apr 16, 2019

Can we merge this please? There are more PRs for Workflow coming to fruition, and if they all get approved/merged at the same time, there will be a number of conflicts...

@javiereguiluz javiereguiluz merged commit 7f3a0fd into symfony:4.2 Apr 17, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
symfony SymfonyCloud: Environment deployed
Details

javiereguiluz added a commit that referenced this pull request Apr 17, 2019

minor #11209 Documented the workflow metadata [redux] (javiereguiluz,…
… pbowyer, OskarStark)

This PR was merged into the 4.2 branch.

Discussion
----------

Documented the workflow metadata [redux]

With @javiereguiluz's permission I have taken #9476 and incorporated the comments and code examples into it.

This is the start of a push to improve the Workflow documentation. The focus now is on expanding the documentation by merging pull requests; organizing it will come later.

Commits
-------

7f3a0fd Oskar's feedback
a4c23c1 Add blank line between code block and sentence above. Code block was not rendering.
6080aa8 Remove the word 'simple'
3765ddb Change code formatting of PHP snippet per https://github.com/symfony/symfony-docs/pull/11209/files/d57fa38d903175d58d9cfbf63f20e7ced8d2fd01#r268391655
55c9199 Indent PHP block by an additional 4 spaces
225c2fe Apply suggestions from code review
f716e81 Incorporate @OskarStark's feedback
94d17de Simplify the English and turn it into a tip
40fbaf3 Update arrays to use short syntax
ea64992 Incorporate further excellent feedback from @HeahDude
af71c14 Add an introduction as suggested in #11209 (comment)
ca66356 Document how to see all configuration options (see #11209 (comment))
c9ef262 Incorporate @OskarStark's feedback
c595c47 First set of tidy-ups for @HeahDude's feedback.
663639b Incorporate the code examples in #9476 (comment) into the documentation
17eae8c Add missing metadata key, fixing comment by @noniagriconomie on #9476
d68cc99 Documented the workflow metadata
@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Apr 17, 2019

Thanks Peter! This has been merged ... and I'll rebase the Workflow doc reorganization (#11437) with your changes.

@OskarStark

This comment has been minimized.

Copy link
Contributor

OskarStark commented Apr 17, 2019

Great work Peter, thank you 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.