added support for serializerGroups #119

Closed
wants to merge 1 commit into
from

Projects

None yet

8 participants

@MaksSlesarenko

No description provided.

Collaborator

This is not mergeable, and it seems completely broken.

Thanks for your work on this, it just needs some tweaks to be perfect :)

Travis is broken because of changes in symfony forms, it has no connection to my changes. I wonder, how it can be not mergeable if my changes were made to your latest version?

Collaborator

Ah, sorry I misread the output.

Collaborator

Then, it's probably good to go :)

William Durand | http://www.williamdurand.fr

On Wed, Dec 19, 2012 at 3:04 PM, MaksSlesarenko notifications@github.comwrote:

Travis is broken because of changes in symfony forms, it has no connection
to my changes. I wonder, how it can be not mergeable if my changes were
made to your latest version?


Reply to this email directly or view it on GitHubhttps://github.com/nelmio/NelmioApiDocBundle/pull/119#issuecomment-11530947.

Nice to hear that.

gionn commented Dec 28, 2012

Any updates on merging this PR?

Collaborator

ping @nelmio

will it be merged?

Collaborator

Ok, so could you rebase your work? And update the tests?

Collaborator
Collaborator

@MaksSlesarenko could you fix the test suite please?

ping @willdurand, are you going to merge this?

Collaborator

Yup, but not now. I need to test it myself.

@ghost
ghost commented Mar 26, 2013

+1

Collaborator

@MaksSlesarenko it would be nice to display the groups in markdown too.

@willdurand should be working for markdown also

Collaborator

@MaksSlesarenko thanks. could you document this feature? A screenshot would be nice.

@willdurand What screenshot or documentation do you need for this small bugfix?
Sorry, man, but this is getting ridiculous, I thought it would be a quickfix, but it is 3 months old now. Maybe, it is just misunderstanding, but this PR feels like waste of time.

Collaborator

Bugfix, really? You want to introduce a new feature. Providing documentation sounds reasonable, period.

If you don't want to do that, no problem, I'll do that myself. But this is not my priority.

This is a bugfix. By default doc shows all fields that are defined in serializer configuration even those fields that would not be displayed by FosRestBundle configuration. My fix correct this output to show only fields that match groups that were defined in serializer to fields that were defined in controller within FosRestBundle.

Should I close this PR? @nelmio

Contributor
stof commented Apr 2, 2013

@MaksSlesarenko This is introducing a new feature: supporting the JMSSerializer serialization groups. And @willdurand asked you to document the feature to have it merged. If you refuse to do it, you will have to wait until he has time to work on the doc. Remmeber that we are all working on these bundles in our free time.

@stof I really don't understand what needs to be documented since this "feature" has no configurations options or visual effects. The only thing it does - prevent from showing fields in response that were not allowed by FosRestBundle. So it is not about JMSSerializer or your bundle itself, it is about FosRestBundle. So right now if we specify groups in FosRestBundle it will respond only those, but the doc will respond all fields.

Here is example of existing code

/**
* @Rest\View(
* serializerGroups={"identity", "user", "detail"}
* )
*
* @ApiDoc(
* resource=true,
* description="Get user identity",
* output="MyCompany\MyBundle\Model\User"
* )
*/

Contributor

+1

I think that a simple sentence to specify that serializerGroupsis supported is enough. We don't really need a screenshot (my POV).

+1

Contributor
skler commented Oct 11, 2013

+1

Collaborator

no doc, no merge.

@willdurand willdurand closed this Nov 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment