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

Path parameter description #189

Merged

Conversation

denisovkiv
Copy link
Contributor

@denisovkiv denisovkiv commented Aug 19, 2020

Closes #188

@chenjr0719 Hello! Answering on feature request #188 May be defining path parameters in consume decorator instead of getting from route parameters is a better idea? Explicit is better than implicit :)

@denisovkiv
Copy link
Contributor Author

denisovkiv commented Aug 22, 2020

@ahopkins

@ahopkins ahopkins requested a review from chenjr0719 August 22, 2020 21:23
@ahopkins
Copy link
Member

I'd love to help, but to be 100% honest, I am only here to help with admin functions. I know very little of the source code here, and am not the one to review and approve the PR. Let's wait to see what @chenjr0719 has to say before I try and look it over.

@opensorceror
Copy link

bump

@artcg
Copy link
Contributor

artcg commented Mar 13, 2021

I like the idea but I think the current method of automatically parsing the path parameters is too valuable to lose,

If this PR was changed to make the manually decorated parameter annotation update the default value with the description instead of overwrite it I think it could be useful, but since the PR is half a year old I think it would be better to close this PR @ahopkins

@denisovkiv
Copy link
Contributor Author

@artcg
Thank you for a reasonable comment!

I just added some improvements. They would save current path param autodetect and override it with decorator.
What do you think?

@artcg
Copy link
Contributor

artcg commented Mar 13, 2021

Cool thanks for the quick change :) I thought this was inactive but great that we can still add it

I think that the route_param = {} addition at L165 is not necessary, so might be good to take that out, but generally this looks good and with this change, I would recommend to @ahopkins that this PR be merged when he has time to go through them ...

@ahopkins ahopkins merged commit 1fd40a7 into sanic-org:master Mar 16, 2021
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.

[Feature Request] Add description to path parameters
4 participants