Skip to content

WIP: HTTP Response Code and Include File #71

Closed
wants to merge 10 commits into from

8 participants

@baldurrensch

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.

baldurrensch added some commits Aug 27, 2012
@baldurrensch baldurrensch Added two new parameters to the ApiDoc Annotation
- '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.
bcd37d5
@baldurrensch baldurrensch Updated README for new functionality bea160a
@baldurrensch baldurrensch and 1 other commented on an outdated diff Aug 28, 2012
Annotation/ApiDoc.php
@@ -12,6 +12,7 @@
namespace Nelmio\ApiDocBundle\Annotation;
use Symfony\Component\Routing\Route;
+use dflydev\markdown\MarkdownParser;
@baldurrensch
baldurrensch added a note Aug 28, 2012

I will add the dependency to the composer.json

@stof
stof added a note Aug 28, 2012

it is already in the composer.json

@stof
stof added a note Aug 28, 2012

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

@baldurrensch
baldurrensch added a note Aug 28, 2012

Oh yeah. just noticed that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@baldurrensch baldurrensch commented on an outdated diff Aug 28, 2012
@@ -196,6 +237,14 @@ input is used, so you can configure their priorities via container tags. Here's
tags:
- {name: nelmio_api_doc.extractor.parser, priority: 2}
+If you want to include a markdown file with long descriptions, you have to set the include `basepath`:
+
+ # app/config/config.yml
+ nelmio_api_doc:
+ include:
+ location: %kernel.root_dir%/../src/
@baldurrensch
baldurrensch added a note Aug 28, 2012

I am not a fan of adding another config variable. However, I don't see any way of getting to the source file from within the Annotation. Therefore, I don't see how else to get the path of the included file. Any ideas?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@travisbot

This pull request fails (merged bea160a into cbdddfe).

@stof
stof commented Aug 28, 2012

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

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

@stof
stof commented Aug 28, 2012

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

I'll look into that. Good idea.

baldurrensch added some commits Aug 28, 2012
@baldurrensch baldurrensch Removed config option for file inclusion
Thanks @stof for tip on how to use ReflectionClass to get the filename. Updated README, and reverted changes to the configuration.
e6a99da
@baldurrensch baldurrensch Moved the rendering to the formatter 496c31b
@travisbot

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

@stof stof and 1 other commented on an outdated diff Aug 28, 2012
Formatter/HtmlFormatter.php
*/
protected function renderOne(array $data)
{
+ if (!empty($data['fileToInclude'])) {
+ if (!is_readable($data['fileToInclude'])) {
+ throw new \InvalidArgumentException("Could not open: {$fileToInclude}");
+ }
+
+ $mdParser = new MarkdownParser();
+ $fileContents = file_get_contents($data['fileToInclude']);
+ $data['fileToInclude'] = $mdParser->transform($fileContents);
+
+ }
@stof
stof added a note Aug 28, 2012

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

@baldurrensch
baldurrensch added a note Aug 28, 2012

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Aug 28, 2012
Resources/views/method.html.twig
+
+ {% if data.responseCodes is defined and data.responseCodes is not empty %}
+ <h4>Response Codes</h4>
+ <table class="fullwidth">
+ <thead>
+ <tr>
+ <th>Response Code</th>
+ <th>Description</th>
+ </tr>
+ </thead>
+ <tbody>
+ {% for response_code, description in data.responseCodes %}
+ <tr>
+ <td><a href="http://en.wikipedia.org/wiki/HTTP_{{ response_code }}" target="_blank">{{ response_code }}<a/></td>
+ <td>{{ description }}</td>
+ {% endfor %}
@stof
stof added a note Aug 28, 2012

the <tr> tag should be closed

@baldurrensch
baldurrensch added a note Aug 28, 2012

Done. /me embarassed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Aug 28, 2012
Resources/views/method.html.twig
@@ -71,8 +91,9 @@
<table>
{% for key, value in infos %}
<tr>
- <td>{{ key|title }}</td>
- <td>{{ value|json_encode|replace({'\\\\': '\\'})|trim('"') }}</td>
+ <td><strong>{{ key|title }}</strong></td>
+ <td>{{ value|replace({'*': ''})|extra_markdown }}</td>
+ {# <td>{{ value|json_encode|replace({'\\\\': '\\'})|trim('"') }}</td>#}
@stof
stof added a note Aug 28, 2012

why changing this ?

@baldurrensch
baldurrensch added a note Aug 28, 2012

I thought that it would be nice to allow Markdown here as well. The strong just added readability. Both reverted back.

@stof
stof added a note Aug 28, 2012

The point is that Markdown is not well suited to dump values. The markdown rendering breaks when the value is an array, and is not always clear for others (some values cannot be distinguished when casted to string)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Aug 28, 2012
Resources/views/method.html.twig
@@ -28,6 +28,26 @@
{% if data.documentation is defined and data.documentation is not empty %}
<h4>Documentation</h4>
<div>{{ data.documentation|extra_markdown }}</div>
+ <div>{{ data.fileToInclude|raw }}</div>
@stof
stof added a note Aug 28, 2012

what about cases where it is not defined ?

@baldurrensch
baldurrensch added a note Aug 28, 2012

Done. Added check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@travisbot

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

@stof stof and 1 other commented on an outdated diff Aug 28, 2012
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>
@stof
stof added a note Aug 28, 2012

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

@baldurrensch
baldurrensch added a note Aug 28, 2012

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@baldurrensch baldurrensch Fixed Twig Filter
Added two small unit tests
79ebe98
@travisbot

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

@baldurrensch

Thanks @stof for all the feedback.

@gordalina gordalina and 1 other commented on an outdated diff Aug 28, 2012
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())) {
@gordalina
gordalina added a note Aug 28, 2012

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

@baldurrensch
baldurrensch added a note Aug 28, 2012

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gordalina

I really like the response codes idea!

@willdurand
Nelmio member

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

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

@willdurand
Nelmio member

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

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

@evillemez

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

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

@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

@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
phidah commented Nov 6, 2012

Any news on this PR?

@baldurrensch

@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
phidah commented Nov 6, 2012
@Seldaek
Nelmio member
Seldaek commented Nov 7, 2012

@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

@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
Nelmio member
Seldaek commented Nov 7, 2012

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.

This was referenced Nov 7, 2012
@willdurand
Nelmio member

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

@baldurrensch

Yes. I'll close it. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.