Skip to content
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

New manifest for NelmioApiDocBundle version 3 #207

Closed

Conversation

unglud
Copy link

@unglud unglud commented Dec 15, 2017

Q A
License MIT

Fixes #197

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

# licenseUrl: null
# cache:
# enabled: false
# file: '%kernel.cache_dir%/api-doc.cache'
Copy link
Contributor

@GuilhemN GuilhemN Dec 17, 2017

Choose a reason for hiding this comment

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

Should be replaced by something like:

nelmio_api_doc:
    documentation:
        info:
            title: My App
            description: This is an awesome app!
            version: 1.0.0
    routes: # to filter documented routes
        path_patterns: # an array of regexps
            - ^/api
            - ^/api/doc$

Copy link

Choose a reason for hiding this comment

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

IMO the default /api/doc route should be excluded from the swagger by defining the path_patterns in a format, something like - ^/api/(?!doc) because currently, it will display /api/doc route in the swagger which is un-needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@takeit you're right, I updated my message

@@ -0,0 +1,3 @@
NelmioApiDocBundle:
resource: "@NelmioApiDocBundle/Resources/config/routing/swaggerui.xml"
prefix: /api/doc
Copy link
Contributor

@GuilhemN GuilhemN Dec 17, 2017

Choose a reason for hiding this comment

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

I guess this should be commented and we should mention that it requires the asset component and the twig bundle.

@Nyholm
Copy link
Member

Nyholm commented Dec 17, 2017

Thank you for this PR.

Im not a big fan of having all the config as a comment. We should add some good default config or remove the file completely. In fact, that config you provide, isn't that good for 90% of the use cases already?

@GuilhemN
Copy link
Contributor

Well we can indeed uncomment it if you prefer :)

@Nyholm
Copy link
Member

Nyholm commented Dec 17, 2017

I think it looks like we can do that. But you know far much more of this bundle than I do. So I direct the decision back to you: Do you think that configuration will be a good start for 90% of the users? Could you make it so?

@GuilhemN
Copy link
Contributor

Do you think that configuration will be a good start for 90% of the users? Could you make it so?

I think it is, it's even what we show first in the bundle's readme.

@Nyholm
Copy link
Member

Nyholm commented Dec 17, 2017

Then go for it =) Remove those comments

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks for working on this, lets wrap it up. can you please check which of the settings are the default value anyways? afaik best practice for the recipes is to not provide configuration with default values that are not likely to be changed. those that are likely to be changed can be in to give a hint. but in the end users are still expected to look at the documentation if they want to customize with configuration. (also, once installed, this file is not updated anymore in users project, so an exhaustive documentation in the recipe files would be counterproductive, as updates will not be seen by users.

# api_version: '0.1'
# info:
# title: Symfony2
# description: 'My awesome Symfony2 app!'
Copy link
Contributor

Choose a reason for hiding this comment

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

flex is symfony 4 mostly, we should just say Symfony instead of Symfony2

# enabled: true
# parameter: _doc
# swagger:
# api_base_path: /api
Copy link
Contributor

Choose a reason for hiding this comment

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

when we uncomment, i guess this should have a comment line above to say what this is.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is 2.0 actually, it does no longer exist in 3.0 :)

@eloyekunle
Copy link

Good day, people,
What is the update on this?

@Nyholm
Copy link
Member

Nyholm commented Dec 28, 2017

I’m waiting on the author to complete the pr. It seams like there was some suggested changes.

@eloyekunle
Copy link

Okay. Thanks.

@Gemorroj
Copy link

Gemorroj commented Jan 3, 2018

@Nyholm It may be better to send a new pull request? Since the author is inactive.

@GuilhemN
Copy link
Contributor

GuilhemN commented Jan 3, 2018

See #226

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants