symfony/symfony-docsPublic

NotificationsYou must be signed in to change notification settings
Fork5.3k
Star2.3k

[WIP] Bootstrapped Options Resolver Component#2353

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

Jump to bottom

Merged

weaverryan merged 4 commits intosymfony:2.1fromwouterj:document_optionresolver

Apr 9, 2013

Merged

[WIP] Bootstrapped Options Resolver Component#2353

weaverryan merged 4 commits intosymfony:2.1fromwouterj:document_optionresolver

Apr 9, 2013

Conversation

Copy link

Member

wouterj commentedMar 24, 2013

Q	A
Doc fix?	yes
New docs?	no
Applies to	2.1+
Fixed tickets	#2030

/cc @bschussek

wouterj added3 commits

March 24, 2013 13:32

[W] components/options_resolver

c5cfa5b

[C] components/options_resolver

89512cf

[R] components/options_resolver

38c61fa

- Removed uses of 'we'

stof reviewed

Mar 24, 2013

View reviewed changes

components/options_resolver.rst Outdated

Copy link

Member

Choose a reason for hiding this comment

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

This is wrong. It cannot be found anywhere in 2.0

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

fixed

[R] components/options_resolver

b1a5bbc

- Review by@stof

weaverryan added a commit that referenced this pull request

Apr 9, 2013

Merge pull request#2353from WouterJ/document_optionresolver

fd8f013

[WIP] Bootstrapped Options Resolver Component

weaverryan merged commitfd8f013 intosymfony:2.1

Apr 9, 2013

weaverryan added a commit that referenced this pull request

Apr 9, 2013

[#2353] Proofreading new options resolver doc

b98619e

Copy link

Member

weaverryan commentedApr 9, 2013

Hi Wouter!

This is a great entry - I've merged it in with only minor changes at sha:b98619e (including removing theversionadded - only because we'll shortly make 2.1 the "oldest-supported" version).

I did have one question - which I'll post as a line comment.

Thanks!

weaverryan reviewed

Apr 9, 2013

View reviewed changes

components/options_resolver.rst

Copy link

Member

Choose a reason for hiding this comment

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

I'm not sure this last part (starting withThe closure that is passed...) is very clear. I think we need an example here to show what you can actually do and how this closure is used.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

I'll add it if you (or anyone) can give me a good example (I can't find a simple good 'somewhat real use-case' example for this one).

Copy link

MemberAuthor

wouterj commentedApr 9, 2013

@weaverryan thanks for merging!
I planned to do some final (english) checking and commit squashing (those are a bit vague now) this week, but I'm happy you had some time improving the english!

However, are we going to remove every.. versionadded:: 2.1 block from the documentation soon? Otherwise, I disagree removing it here

Copy link

Contributor

webmozart commentedApr 19, 2013

Thank you for this great documentation! In my opinion, the example should be changed though. Entities are a very bad usage example, because entities are data holders that can change at any time. The OptionsResolver though is meant for configuring classes whose configuration does not change anymore after construction.

A better example might be aMailer class for example.