Movatterモバイル変換

Copy link

Contributor

OskarStark left a comment

Choose a reason for hiding this comment

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

Thank you for working on this 👍

service_container/tags.rst OutdatedShow resolvedHide resolved

alexandrunastase force-pushed theadd-method-priority-for-tagged-services branch 2 times, most recently from217e1ac to2b5bcd4Compare

December 1, 2019 19:46

alexandrunastase marked this pull request as ready for review

December 1, 2019 19:48

alexandrunastase changed the title~~[WIP] [DependencyInjection] Add docs for method priority for tagged services~~[DependencyInjection] Add docs for method priority for tagged services

Dec 1, 2019

alexandrunastase changed the title~~[DependencyInjection] Add docs for method priority for tagged services~~[DependencyInjection] Add docs for default method priority for tagged services

Dec 1, 2019

alexandrunastase changed the title~~[DependencyInjection] Add docs for default method priority for tagged services~~[DependencyInjection] Add docs for default priority method for tagged services

Dec 1, 2019

OskarStark reviewed

Dec 3, 2019

service_container/tags.rst Outdated

		}

		..tip::
		Prioritizing Tagged Services

Copy link

Contributor

Choose a reason for hiding this comment

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

Maybe „Tagged Services with Priority“ sounds more natural?

cc@javiereguiluz

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

For me the current one sounds a bit clearer, but if others think that "Tagged Services with Priority" is better I will just update it.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Changed it to how you suggested

service_container/tags.rst Outdated

		Prioritizing Tagged Services
		~~~~~~~~~~~~~~~~~~~~~~~~~~~~

		The tagged services can be prioritized using the ``priority`` attribute:

Copy link

Contributor

Choose a reason for hiding this comment

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

To me a sentence is missing what I can achieve by using priority. I mean we are talking a lot about the how but not the why. A real world use case would be very helpful to get context. Thanks

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

From my side it looks pretty obvious, but maybe I am not the best person to ask since I did not need this until now. I started looking into this on Symfony Hack Day. If you can share some use case, we can surely added it.

Copy link

Contributor

HeahDudeFeb 19, 2020

Choose a reason for hiding this comment

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

What about expanding that sentence a little? Something like:

The tagged services can be prioritized using the ``priority`` attribute,so a consumer can be injected a sorted collection:

Copy link

ContributorAuthor

alexandrunastaseMar 22, 2020

Choose a reason for hiding this comment

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

Updated. I tried to avoid using the wordconsumer since that can have different meanings and might confuse people. Thanks@HeahDude

HeahDude suggested changes

service_container/tags.rst

Copy link

Contributor

HeahDude left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Some minor comments

service_container/tags.rst Outdated

		Prioritizing Tagged Services
		~~~~~~~~~~~~~~~~~~~~~~~~~~~~

		The tagged services can be prioritized using the ``priority`` attribute:

Copy link

Contributor

HeahDudeFeb 19, 2020

Choose a reason for hiding this comment

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

What about expanding that sentence a little? Something like:

The tagged services can be prioritized using the ``priority`` attribute,so a consumer can be injected a sorted collection:

		..tip::
		Prioritizing Tagged Services
		~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link

Contributor

HeahDudeFeb 19, 2020

Choose a reason for hiding this comment

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

We need a.. versionadded:: 4.4 here, seehttps://symfony.com/doc/current/contributing/documentation/format.html#new-features-behavior-changes-or-deprecations.

Copy link

ContributorAuthor

alexandrunastaseMar 22, 2020

Choose a reason for hiding this comment

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

Added it. Thanks :)

Copy link

Contributor

HeahDudeApr 16, 2020

Choose a reason for hiding this comment

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

Missing a dot :)

Copy link

ContributorAuthor

alexandrunastaseJun 8, 2020•
edited
Loading

Choose a reason for hiding this comment

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

Added it :)

service_container/tags.rst OutdatedShow resolvedHide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels

HeahDude modified the milestones:4.3,4.4

HeahDude linked an issue

[DependencyInjection] added Ability to define a priority method for tag…#12406

that may beclosed by this pull request

Closed

alexandrunastase force-pushed theadd-method-priority-for-tagged-services branch from2b5bcd4 tob2fd0f1Compare

March 22, 2020 09:11

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Mar 22, 2020

alexandrunastase force-pushed theadd-method-priority-for-tagged-services branch 3 times, most recently froma21981b tod5b8517Compare

March 22, 2020 09:21

HeahDude suggested changes

Apr 8, 2020

Copy link

Contributor

HeahDude left a comment

Choose a reason for hiding this comment

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

One last change here and we should be good. Thanks

service_container/tags.rst OutdatedShow resolvedHide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review Status: Needs Work labels

Apr 8, 2020

carsonbot added the Status: Needs Review label

Apr 15, 2020

HeahDude approved these changes

Apr 16, 2020

service_container/tags.rst

Copy link

Contributor

HeahDude left a comment

Choose a reason for hiding this comment

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

Thanks for documenting this. I've spotted 2 typos on my last review, but we can handle them on merge ✌️.

		..tip::
		Prioritizing Tagged Services
		~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link

Contributor

HeahDudeApr 16, 2020

Choose a reason for hiding this comment

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

Missing a dot :)

service_container/tags.rst Outdated

		->args([
		tagged_iterator('app.handler', null, null, 'getPriority'),
		]
		);

Copy link

Contributor

HeahDudeApr 16, 2020

Choose a reason for hiding this comment

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

Semi colon on next line

Copy link

ContributorAuthor

alexandrunastaseJun 8, 2020•
edited
Loading

Choose a reason for hiding this comment

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

Fixed the semicolon. Not sure about the dot. Assumed it was at the end of the versionadded sentence.

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Apr 16, 2020

alexandrunastase force-pushed theadd-method-priority-for-tagged-services branch fromf5da791 to4c99d1fCompare

June 8, 2020 06:37

maxhelias approved these changes

Jun 14, 2020

[DependencyInjection] Add docs for default priority method for tagged…

294ac51

… services

wouterj force-pushed theadd-method-priority-for-tagged-services branch from4c99d1f to294ac51Compare

October 3, 2020 21:15

wouterj added a commit that referenced this pull request

[#12697] Some minor tweaks

8a7d32e

wouterj merged commitb7a35f2 intosymfony:4.4

Copy link

Member

wouterj commentedOct 3, 2020

I'm sorry that this PR has been open for soo long@alexandrunastase. At least, it's merged now. I've done some very minor tweaks afterwards in8a7d32e . Thanks for adding the documentation!

wouterj added a commit that referenced this pull request

Merge branch '4.4' into 5.1

7a820e3

* 4.4:  [#12697] Some minor tweaks  [DependencyInjection] Add docs for default priority method for tagged services  [#12461] Added versionadded directive  List CSV encoder's context options

wouterj added a commit that referenced this pull request