Skip to content

Finished #3886 #4237

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

Merged

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Sep 16, 2014

Replaces #3886

@weaverryan
Copy link
Member

@wouterj I want to make sure I get this merge correct: this should apply to only the 2.3 branch, correct? And so when this merges into 2.4, we'll need to revert it there.

@@ -83,7 +83,7 @@ The ``FormEvents::PRE_SET_DATA`` event is dispatched at the beginning of the
.. sidebar:: ``FormEvents::PRE_SET_DATA`` in the Form component

The ``collection`` form type relies on the
:class:`Symfony\\Component\\Form\\Extension\\Core\\EventListener\\ResizeFormListener`
``Symfony\Component\Form\Extension\Core\EventListener\ResizeFormListener``
Copy link
Member

Choose a reason for hiding this comment

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

@@ -152,10 +152,10 @@ It can be used to:

.. sidebar:: ``FormEvents::PRE_SUBMIT`` in the Form component

The :class:`Symfony\\Component\\Form\\Extension\\Core\\EventListener\\TrimListener`
The ``Symfony\Component\Form\Extension\Core\EventListener\TrimListener``
Copy link
Member

Choose a reason for hiding this comment

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

@xabbuh
Copy link
Member

xabbuh commented Sep 19, 2014

@weaverryan That's right. The versionadded directives for the data collector extension already have been added in #3887.

@wouterj
Copy link
Member Author

wouterj commented Sep 19, 2014

@xabbuh I decided to remove all class directives in the sidebar, to be consistent and they don
don't add any value

@xabbuh
Copy link
Member

xabbuh commented Sep 19, 2014

@wouterj I understand your concerns. Though having fully qualified class names rendered in the documentation looks pretty ugly to me. That's why I'd prefer the class here in favour of putting the FQCN in literals.

@wouterj
Copy link
Member Author

wouterj commented Sep 20, 2014

@xabbuh tbh, I don't like to use the :class: API links everywhere in the documentation. In most cases, they don't add any real value. The only good thing about them, is that it allows us to "hide" the FQCN, but at the same time making it also available on hover.

I think we should make a new directive which does only that, without the link.

@xabbuh
Copy link
Member

xabbuh commented Sep 20, 2014

Of course, building such a directive should be very easy since we could reuse most of the code from the class directive. Though, what do you dislike of the the API links?

@wouterj
Copy link
Member Author

wouterj commented Sep 20, 2014

@xabbuh let's take this case as an example. The class shows as an implementation example of the listener for a specific event. I don't care what the signature of the class is (in fact, everyone with a basic event system knowledge knows that it has a getSubscribedEvents method, another method and properbly a constructor), I care about the implementation. If there is anything the classname should link to, it should be the github code.

@weaverryan
Copy link
Member

I agree. In this case, we're saying "hey, look at this core code so you can learn from it!" - so in this case, the API docs don't help (and like Wouter said, a link to the GH source code if anything would be appropriate - perhaps having an extension to make this link with the proper version would be nice). Anyways, I'm +1 for what we've done here, and in general, we'll just be thoughtful whenever we have a class :).

@weaverryan weaverryan merged commit 01057ae into symfony:2.3 Oct 2, 2014
weaverryan added a commit that referenced this pull request Oct 2, 2014
This PR was merged into the 2.3 branch.

Discussion
----------

Finished #3886

Replaces #3886

Commits
-------

01057ae Reverted removal, removed API links instead
0670f25 [2.3] Examples that points to the DataCollectorListener should be removed.
@wouterj wouterj deleted the ahsio-fix-added-example-to-wrong-version branch October 2, 2014 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants