Fix Issue 7924 - Allow host and base path to be changed independently #7996
Conversation
|
Tests seem to pass, so not sure what I've done to make the CI fail now... Is there anything else I need to do to move this forward? (not sure who to tag, @jeff9finger?) |
|
I like the concept. However, I think this would be more useful to provide more granularity here. Combining the schema, host and port into a property named "host" is a bit misleading. I'd like to see separate properties for schema, host, and port (or at least schema and host with the port combined with the host). An alternative would be to rename the property to something besides "host". What does the community think? |
|
Was erring on the side of making small changes, but agree it would make sense to split out the scheme as well and am happy to do that 🙂. (Alternatively, if there's already some precedent for this in swagger-codegen, then I'm also happy to follow that.) |
|
Separated out the scheme, let me know if there's anything else you want from me. |
| @@ -53,7 +53,9 @@ import {{invokerPackage}}.auth.OAuth; | |||
|
|
|||
| public class ApiClient { | |||
|
|
|||
| private String basePath = "{{{basePath}}}"; | |||
| private String scheme = "{{scheme}}"; | |||
There was a problem hiding this comment.
Shouldn't this provide a default for "http" in a similar way that host does with "localhost"
There was a problem hiding this comment.
If I understand how this works correctly, then I believe this defaults to https if a scheme isn't specified (https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/java/io/swagger/codegen/DefaultGenerator.java#L98).
There was a problem hiding this comment.
Yes. that is correct. Thanks for pointing that out.
|
Also, this seems useful for all the java client libraries. Would you consider adding it to the other Java client libraries? |
|
My project happens to use this library so that's what I've been testing with. I'd rather not edit the other Java client libraries since there are at least some where it looks my change won't easily apply, and I don't want to touch more code that I don't have time to test right now! |
|
@jeff9finger Do you know if this is going to be merged soon, or are you planning to wait for the other libraries to be updated? |
|
@wing328 Can we go ahead and get this merged? |
|
@wing328 bump? |
|
Any updates? |
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\.3.0.0branch for changes related to OpenAPI spec 3.0. Default:master.@bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger
Description of the PR
Fixes #7924
Separate the hostname and the base path so that they can be changed independently of one another