Added versioning support #16
Conversation
There was a problem hiding this comment.
Hi,
Great thanks for your contribution.
I have a few minor comments. Coming from our coding style. Please check it (Review changes).
I worry about breaking change.
It would be nice to support the original configuration. But it's from category 'nice to have'.
Unit tests
Now is time write first tests for configuration. Since they have not been tested so far, I create new issue.
| @@ -12,6 +12,6 @@ public class SwaggerForOcelotUIOptions: SwaggerUIOptions | |||
| /// The end point base path. The final path to swagger endpoint is | |||
| /// <see cref="EndPointBasePath"/> + <see cref="SwaggerEndPointOptions.Key"/> | |||
| /// </summary> | |||
| public string EndPointBasePath { get; set; } = "/swagger/v1"; | |||
| public string EndPointBasePath { get; set; } | |||
There was a problem hiding this comment.
Add the default value (/swagger/docs) for property EndPointBasePath.
In my opinion, for most scenarios, it is unnecessary to configure property EndPointBasePath.
| /// <returns> | ||
| /// <see cref="IApplicationBuilder"/>. | ||
| /// </returns> | ||
| public static IApplicationBuilder UseSwaggerForOcelotUI( |
There was a problem hiding this comment.
Why did you remove this overload?
In my opinion, for most scenarios, it is unnecessary to configure property EndPointBasePath.
| @@ -43,11 +30,10 @@ public static class BuilderExtensions | |||
| setupAction?.Invoke(options); | |||
|
|
|||
| UseSwaggerForOcelot(app, options); | |||
|
|
|||
| @@ -0,0 +1,55 @@ | |||
| namespace OrderService | |||
| { | |||
| using Microsoft.AspNetCore.Mvc.ApiExplorer; | |||
There was a problem hiding this comment.
Nit: we usually have the using section at beginning of document.
demo/OrderService/Startup.cs
Outdated
| @@ -0,0 +1,104 @@ | |||
| namespace OrderService | |||
| { | |||
| using Microsoft.AspNetCore.Builder; | |||
There was a problem hiding this comment.
Nit: we usually have the using section at beginning of document.
| foreach (var config in endPoint.Config) | ||
| { | ||
| c.SwaggerEndpoint($"{basePath}/{config.Version}/{endPoint.KeyToPath}", $"{config.Name} - {config.Version}"); | ||
| } |
There was a problem hiding this comment.
Nit: please remove whitespaces after }.
Tip: we use extension Trailing Whitespace Visualizer.
| var url = endPoint.Config.Where(x => x.Version == endPointInfo.Version).FirstOrDefault()?.Url; | ||
| return (url, endPoint); | ||
| } | ||
| /// <summary> |
There was a problem hiding this comment.
Nit: by our coding style, we adding one empty line beatween methods.
| private (string Version, string Key) GetEndPointInfo(string path) | ||
| { | ||
| var keys = path.Split('/'); | ||
| return (keys[1], keys[2]); |
There was a problem hiding this comment.
Nit: please remove whitespaces.
Tip: we use extension Trailing Whitespace Visualizer.
|
Hi, Thanks for the quick reply, i will do the necessary! |
|
I will add configuration unit tests later! |
|
Hi, Release come in a few minits, v1.1.0 Thank you again. |
The RP add versioning support.
New Config :
In the previous implementation, you could not add multiple routes for a specific API. This change addresses that problem to allow adding multiples configuration for a swagger endpoint.