WIP: HTTP Response Code and Include File #71

baldurrensch · 2012-08-28T16:24:57Z

Hey all,

this is a PR. It's currently WIP, so, Tests are not added, and not all templates are up to date. I wanted to get some feedback first (and whether there is any interest for that). Basically it adds two new parameters to the ApiDoc annotation. I updated the README to show how to use them.

I noticed that "response" was just merged in, however I believe that my response codes is different. Any feedback is appreciated.

- 'responseCodes' allows to define the HTTP response codes and the situations when they are returned. (Think of Docblocks and Exceptions). - 'include' allows to include a Markdown file with a long description. - Minor tweaks: Made the Filters actually display as Markdown as well. - When 'default' is included in filters, use this instead of the description for the sandbox.

baldurrensch · 2012-08-28T16:25:22Z

Annotation/ApiDoc.php

@@ -12,6 +12,7 @@
 namespace Nelmio\ApiDocBundle\Annotation;

 use Symfony\Component\Routing\Route;
+use dflydev\markdown\MarkdownParser;


I will add the dependency to the composer.json

it is already in the composer.json

however, the annotation should not be responsible of the parsing. The rendering should

Oh yeah. just noticed that.

travisbot · 2012-08-28T16:32:04Z

This pull request fails (merged bea160a into cbdddfe).

stof · 2012-08-28T16:34:54Z

btw, this included file has nothing to do in the ApiDoc annotation: you are defining it as a global setting, whereas the annotation is about one route

baldurrensch · 2012-08-28T16:38:11Z

I don't understand your last comment. The setting is simply a basepath. The annotation is still about one route.

stof · 2012-08-28T16:40:49Z

hmm, indeed.

But this looks weird. Couldn't you make the path relative to the location of the controller (which can be retrieved from the reflection objects) ? Thsi would avoid having to configure a base path, making it possible for shared bundles to use it if they want too.

baldurrensch · 2012-08-28T16:42:10Z

I'll look into that. Good idea.

@stof

Thanks @stof for tip on how to use ReflectionClass to get the filename. Updated README, and reverted changes to the configuration.

travisbot · 2012-08-28T17:33:33Z

This pull request fails (merged 496c31b into cbdddfe).

stof · 2012-08-28T17:55:15Z

Formatter/HtmlFormatter.php

+            $fileContents = file_get_contents($data['fileToInclude']);
+            $data['fileToInclude'] = $mdParser->transform($fileContents);
+
+        }


why not simply using the |markdown filter to do the rendering part, passing the content of the file here ?

Also check whether included file is defined and non-empty

travisbot · 2012-08-28T18:43:59Z

This pull request fails (merged 4d7f9bf into cbdddfe).

stof · 2012-08-28T18:47:21Z

Resources/views/method.html.twig

@@ -28,6 +28,29 @@
            {% if data.documentation is defined and data.documentation is not empty %}
                <h4>Documentation</h4>
                <div>{{ data.documentation|extra_markdown }}</div>
+                {% if data.fileToInclude is defined and data.fileToInclude is not empty %}
+                <div>{{ data.fileToInclude|markdown }}</div>


the filter defined in the bundle is extra_markdown, not markdown (sorry for giving a wrong name)

Added two small unit tests

travisbot · 2012-08-28T19:07:14Z

This pull request passes (merged 79ebe98 into cbdddfe).

baldurrensch · 2012-08-28T19:09:02Z

Thanks @stof for all the feedback.

gordalina · 2012-08-28T19:12:33Z

Extractor/ApiDocExtractor.php

@@ -305,6 +305,12 @@ protected function extractData(ApiDoc $annotation, Route $route, \ReflectionMeth
        $annotation->setMethod($route->getRequirement('_method') ?: 'ANY');
        $annotation->setUri($route->getPattern());

+        // Include File
+        if ("" != ($fileToInclude = $annotation->getFileToInclude())) {


if (!($fileToInclude = $annotation->getFileToInclude())) {
or
if (false === ($fileToInclude = $annotation->getFileToInclude())) {
or
if (empty($fileToInclude = $annotation->getFileToInclude())) {
look cleaner to me

gordalina · 2012-08-28T19:13:28Z

I really like the response codes idea!

willdurand · 2012-08-28T19:33:12Z

I don't get the point of the response codes. First, it would be named "status codes" but also, why do you want to add these information in the annotation? You can write complete documentation in the doc block.

I like to document my status codes too, but I often need to provide the response body with a given status code.

travisbot · 2012-08-28T19:34:39Z

This pull request passes (merged 88ace78 into cbdddfe).

willdurand · 2012-08-28T19:35:46Z

Also, I thought a lot about your second feature, and having documentation in another file leads to outdated documentation as you can change the code without changing the documentation (because you missed the included file). However, it would fix a common issue: the huge doc blocks in the controllers.

baldurrensch · 2012-08-28T19:36:51Z

Right, it's one of those things where both kind of suck. :-/

evillemez · 2012-08-28T19:49:22Z

I think these two things should be separate PRs.

I like responseCodes, I was going to make at a stab at that for documenting errors with #65, but I think this is a better approach because you can take into account things that aren't actually errors. But, I agree with willdurand that "statusCodes" would be better... and I'm thinking that maybe there should be other ways of providing the information besides just declaring it explicitly in the @ApiDoc annotation. For example, @throws annotations are often use to document error conditions.

include, or something like it, could be good in theory, as the @ApiDoc annotation is getting fairly large. But I'm not sure about using markdown as the format... it seems to me that the list of filters and such is probably the longest part of the documentation (if provided). The biggest issue I see is that it would be pain to have to include Markdown and then parse it into a data structure to use for a formatter that's going to output JSON, XML, or some other data format. The formatters should already be receiving parsed data structures from the ApiDocExtractor so they don't have to deal with both parsing data and formatting data.

baldurrensch · 2012-08-28T20:20:14Z

Regarding responseCodes, I don't see a problem with adding (optional) response bodies - I actually like that idea. I agree that they should be renamed to "statusCodes". Regarding using @throws annotations, now things get a little more complicated. For example, how do you get the status code from the exception (especially if they are not mapped with the FOSRestBundle). But, this could be added later.

include seems to really start a big controversy. It therefore seems that really only the statusCodes (aka responseCodes) part should be moved into a new PR. If at all. But I only want to do that if there is real interest.

evillemez · 2012-08-28T21:12:57Z

@baldurrensch You're right about the @throws... thinking about actually implementing it makes me realize how gross it would be.

For what it's worth, here is how Swagger documents errors, which would be a subset of responses:

https://github.com/wordnik/swagger-core/wiki/errors

As it stands in this PR, it would be easy for me to check specifically for 400-500 range errors to support Swagger using the response data documented here. The response body content its self is already covered with with the return property... though it may need some updates to make it more robust. If the response body content is different when there's an error encountered, then... that's going to get tricky to document.

@asm89 mentioned in IRC splitting the @ApiDoc annotation into multiple annotations. If we're going to go nuts on this statusCodes property, and entertain the idea of an include property because of having a growing annotation, we may as well discuss the approach sooner rather than later.

baldurrensch · 2012-08-28T21:30:54Z

@evillemez The response body is -- in almost all cases -- different from the success scenario, I would think.

I definitely like the idea of splitting up the @ApiDoc annotation in multiple ones. I think it would make it way more manageable, so 👍 from me.

By the way, swagger calls the statusCodes: errorResponses.code...

phidah · 2012-11-06T20:46:27Z

Any news on this PR?

baldurrensch · 2012-11-06T21:18:40Z

@phidah Unfortunately I never got a clear answer whether my PR was desired or not, or whether I should split it up in multiple parts, so I put it to sleep. :-(

phidah · 2012-11-06T21:19:44Z

That seems to be a general problem with the bundles from Nelmio :(

On Tuesday den 6. November 2012 at 22.18, Baldur Rensch wrote:

@phidah (https://github.com/phidah) Unfortunately I never got a clear answer whether my PR was desired or not, or whether I should split it up in multiple parts, so I put it to sleep. :-(

—
Reply to this email directly or view it on GitHub (#71 (comment)).

Seldaek · 2012-11-07T21:41:33Z

@phidah sorry but we're a small team and I already spend a lot of time on other stuff like composer/packagist, so we do what we can here.

@baldurrensch Anyway I just reviewed this quickly and as it was said above I agree it should be split in two PRs. I'm pretty much ok with the response codes bit and would be happy to merge that. My only comment there would be that statusCodes might be better than responseCodes, after all that's what the spec calls them.

Regarding the file includes, I'm still not sure if that's a good idea. It definitely goes against the point of this bundle that is to keep everything in your code to avoid stuff getting too out of date. Then again one might argue it's ok to give the freedom to shoot yourself in the foot to users. Just not sure what the use case is?

baldurrensch · 2012-11-07T21:45:55Z

@Seldaek: Perfect, I will split it up in the next few days, as soon as I find some time. I see the point of enforcing the documentation to be in the source code file, but that can blow those up in some cases when you need/want a lot of documentation.

Seldaek · 2012-11-07T21:52:34Z

For the includes, one approach that might be better is to do a {@include ...} or whatever syntax phpdoc might have (if it does, otherwise make one up) inside the description itself rather than as a specific parameter of @ApiDoc That way you would have a) better phpdoc, and b) ability to include just parts of a large description, for repetitive stuff like data types explanations you could build that in external files that you include in all methods that need it.

willdurand · 2012-11-13T17:08:35Z

Can we close that PR? If you want to continue the discussion about include, you should open a RFC.

baldurrensch · 2012-11-13T17:11:48Z

Yes. I'll close it. Thanks.

baldurrensch and others added 2 commits August 28, 2012 09:18

Updated README for new functionality

bea160a

baldurrensch reviewed Aug 28, 2012
View reviewed changes

Baldur Rensch added 2 commits August 28, 2012 10:17

Removed config option for file inclusion

e6a99da

Thanks @stof for tip on how to use ReflectionClass to get the filename. Updated README, and reverted changes to the configuration.

Moved the rendering to the formatter

496c31b

Add File Inclusion to Markdown dump

5ea0f20

stof reviewed Aug 28, 2012
View reviewed changes

Baldur Rensch added 3 commits August 28, 2012 11:03

Replaced explicit Markdown parsing with Twig builtin one

bc73835

Also check whether included file is defined and non-empty

Added forgotten closing tag

dc1c91e

Reverted a previous change

4d7f9bf

stof reviewed Aug 28, 2012
View reviewed changes

Fixed Twig Filter

79ebe98

Added two small unit tests

gordalina reviewed Aug 28, 2012
View reviewed changes

Made the code more clear

88ace78

evillemez mentioned this pull request Nov 7, 2012

Add errors property to ApiDoc #65

Closed

baldurrensch mentioned this pull request Nov 13, 2012

Added status codes #93

Merged

baldurrensch closed this Nov 13, 2012

WIP: HTTP Response Code and Include File #71

WIP: HTTP Response Code and Include File #71

Conversation

baldurrensch commented Aug 28, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisbot commented Aug 28, 2012

stof commented Aug 28, 2012

baldurrensch commented Aug 28, 2012

stof commented Aug 28, 2012

baldurrensch commented Aug 28, 2012

travisbot commented Aug 28, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisbot commented Aug 28, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisbot commented Aug 28, 2012

baldurrensch commented Aug 28, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gordalina commented Aug 28, 2012

willdurand commented Aug 28, 2012

travisbot commented Aug 28, 2012

willdurand commented Aug 28, 2012

baldurrensch commented Aug 28, 2012

evillemez commented Aug 28, 2012

baldurrensch commented Aug 28, 2012

evillemez commented Aug 28, 2012

baldurrensch commented Aug 28, 2012

phidah commented Nov 6, 2012

baldurrensch commented Nov 6, 2012

phidah commented Nov 6, 2012

Seldaek commented Nov 7, 2012

baldurrensch commented Nov 7, 2012

Seldaek commented Nov 7, 2012

willdurand commented Nov 13, 2012

baldurrensch commented Nov 13, 2012