yahehe / Nancy.Swagger

Nancy plugin for generated API documentation in Swagger format.
MIT License
133 stars 60 forks source link

RouteParamAttribute enhancements. #155

Closed vincentparrett closed 6 years ago

vincentparrett commented 6 years ago

The RouteParamAttribute could use some enhancements allow the following :

  1. Usage on Methods - code looks quite messy when there are lots of parameter attributes, hard to read, so it might just be better to allow adding to the method instead (especically for query params, where they may be pulled from the Request object later and not passed as method parameters).
  2. Edit - turns out I can specify the description & required so no need for new constructor props.

I'm willing to submit a pull request for these.

jnallard commented 6 years ago

I'm not exactly sure what you mean.

Can you show the problem and an example of the proposed solution?

vincentparrett commented 6 years ago

I currently have this scenario :

private BuildsListModel GetBuildsPaged([RouteParam(ParameterIn.Query, "BuildSummaryStatus", Required = true, Description = "Long string with a description of the parameter")] [RouteParam(ParameterIn.Query, "Skip", Description = "Long string with a description of the parameter")] [RouteParam(ParameterIn.Query, "Take", Description = "Long string with a description of the parameter")]  Paging paging)

Allowing the RouteParam attribute to be specified on the method would give me this :

[RouteParam(ParameterIn.Query, "BuildSummaryStatus", Required = true, Description = "Long string with a description of the parameter")]
[RouteParam(ParameterIn.Query, "Skip", Description = "Long string with a description of the parameter")] 
[RouteParam(ParameterIn.Query, "Take", Description = "Long string with a description of the parameter")]  
private BuildsListModel GetBuildsPaged(Paging paging)

The latter is much easier to read.

jnallard commented 6 years ago

You can make it a bit easier to read by placing the different attributes on different lines. (I assume)

private BuildsListModel GetBuildsPaged (
    [RouteParam(ParameterIn.Query, "BuildSummaryStatus", Required = true, Description = "Long string with a description of the parameter")] 
    [RouteParam(ParameterIn.Query, "Skip", Description = "Long string with a description of the parameter")]
    [RouteParam(ParameterIn.Query, "Take", Description = "Long string with a description of the parameter")]  Paging paging
)

I'm not sure how I feel about this idea. I like the attributes being directly applied to the parameters, but you did add a way to have attributes that aren't actually tied to the parameter, so this makes more sense now. Feel free to try and implement it, and if you can do it cleanly, I'll probably approve it.

vincentparrett commented 6 years ago

Yes, wrapping the lines works, but the reality is that the attributes have zero runtime impact, so really it doesn't matter where they live (method or parameters). My changes this week allow you to do things like document optional query strings that don't have a parameter in the handler method (ie we test for them in the handler method from the Request object).

I have it coded here, but need to play with it a bit and see. When it's used on the method, the parameter type cannot be inferred from the parameter, so it needs to be specified on the attribute.. that's something I missed with the multiple routeparam attribute support which I also need to figure out.

yahehe commented 6 years ago

Your suggestion feels more natural to me Vincent, I'm surprised it can't work that way now. If you make a PR I would approve it