#4241, #4236 Add cutom Json serializer and proxy settings to config #4466
Conversation
OpenAPITools#4236 Add proxy settings so config
|
@mandrean (2017/08), @jimschubert (2017/09) @frankyjuang (2019/09) |
|
How I can rebuild CircleCI? I generated some pedstore things.. |
|
@PeterTriebe Thanks for the contribution! I've triggered the build for you. |
| @@ -223,6 +225,13 @@ namespace {{packageName}}.Client | |||
| /// <value>The access token.</value> | |||
| public virtual string AccessToken { get; set; } | |||
|
|
|||
|
|
|||
| /// <inheritdoc /> | |||
| public JsonSerializerSettings SerializerSettings { get; set; } = new JsonSerializerSettings(); | |||
There was a problem hiding this comment.
I think this will need to be moved elsewhere… maybe as a constructor argument for ApiClient or as a settable property (or both?).
Configuration should be decoupled from framework implementation. For example, ApiClient is tightly coupled to RestSharp and JSON.net. Having Configuration rely only on "standard" types means that users may replace ApiClient with their desired accessor implementations. Including Json.NET types in Configuration will make this unusable for users who don't pull that type into their client.
There was a problem hiding this comment.
Its possible to move it to the constructor. But everything is configurated inside the Configuration type but just not this? I think every C# developer use Newtonsoft.Json. Even if the ApiClient changes. When it moved to the constructor of the ApiClient , the class that wrap this must also use Newtonsoft.Json.
If you want to use this type just inside the ApiClient , we have to create a custom type and map everything. But this is not a good way.
| /// <summary> | ||
| /// Gets the json serializer settings | ||
| /// </summary> | ||
| JsonSerializerSettings SerializerSettings { get; } |
There was a problem hiding this comment.
See previous comment about scoping this to ApiClient only.
| /// <summary> | ||
| /// Gets the proxy | ||
| /// </summary> | ||
| WebProxy Proxy { get; } |
There was a problem hiding this comment.
Note: changing this interface is a breaking change for existing client consumers.
Also, formatting is off by 4 spaces too many here.
There was a problem hiding this comment.
I thought this interface is just used from the Configuration ? And this class is also generated. Using this proxy is optional
There was a problem hiding this comment.
Since 5.0 is released soon, I think the breaking change here will be fine for client release. I still think the JsonSerializerSettings should stay in ApiClient, however, because this isn't API configuration but an implementation detail of ApiClient. Since we allow for users to define their own ApiClients (and bring their own serializers), it doesn't make a lot of sense to put ApiClient implementation-specific details into the Configuration object. That is, this configuration object should refer to System namespaces only.
I'd be happy to resolve this in a separate commit.
|
@PeterTriebe I've left a few comments. CircleCI has failed because you'll need to rebuild the project and run the relevant csharp-netcore scripts under |
|
@jimschubert I have run the scripts and pushed it after the run fails the first time. Hm. I can try it again |
|
Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors. Let me know if you need help fixing it. |
|
is the 4.2.3 milestone correct? does the release 4.2.3 contain this pull request? |
|
@wing328 the author of this PR hasn't responded to the request to tie the commits to the GitHub profile, and the implementation still needs a little work to follow the client's patterns. Namely, some formatting needs to be fixed and the NewtonSoft settings should be moved out of the configuration object into the ApiClient implementation. Is it acceptable to fix this via separate PR and merge with the author details as-is? |
#4236 Add proxy settings to config
This was needed because in net-core 2x its not possible to set a global proxy.
#4241 Add json serializer settings to config
This was needed to change the serializer settings. In my case i wanted to ignore null entries.
PR checklist
./bin/(or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run./bin/{LANG}-petstore.sh,./bin/openapi3/{LANG}-petstore.shif updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).master,4.3.x,5.0.x. Default:master.