-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
C# template refactor #737
C# template refactor #737
Conversation
@jimschubert |
* master: (32 commits) Fixed date formatting in typescript node client (OpenAPITools#786) better explain usage (OpenAPITools#794) Fix float/double default value in C# generator (OpenAPITools#791) Enhancements to documentation generators (samples, default values, etc) (OpenAPITools#790) Remove duplicate variable declaration (OpenAPITools#792) Issue 758 root resource (OpenAPITools#771) Do not declare destructor as default when destructor is explicitly declared. (OpenAPITools#732) Fix C# client enum issue (OpenAPITools#774) [JavaScript] Update vulnerable dependencies (OpenAPITools#784) [Ruby] Fix method split (OpenAPITools#780) [Java][jaxrs-jersey] add sample with jaxrs-jersey + openapi v3 (OpenAPITools#778) update groupId in pom (OpenAPITools#779) [cpp-restsdk] Support multi-line descriptions (OpenAPITools#753) [Core] Resolve Inline Models (OpenAPITools#736) [gradle] Support nullable system property values (OpenAPITools#764) Correct URL for openapi-generator.cli.sh in README.md (OpenAPITools#770) Fixed the generation of model properties whose data type is a composed (allOf) schema (OpenAPITools#704) [JAX-RS][Spec] Add samples to CircleCI (OpenAPITools#759) minor update to python generator usage (OpenAPITools#762) [C++][Restbed/Pistache] Added fix for byte array (OpenAPITools#752) ...
The enum issue I found was fixed by #774, so I've merged master and regenerated the C# client. Tests in the sample(s) would need to be updated to this new structure, but I'll await feedback on the approach beforehand. Below are callouts about things done in this refactor. Interfaces for sync/async client codeA major point of issue for consumers in the past has been our hard reliance on RestSharp, which required modifying templates to undo the coupling. To address this, I created some abstractions around a REST client. ApiResponse (generic)Any client implementation may return a generic IAsynchronous ClientAll methods return ISynchronous ClientAll methods return ApiClientFunctionality that previously tightly coupled us to RestSharp has been encapsulated into ApiClient, reducing the visibility of anything RestSharp-specific and exposing only the interfaces above. ApiClient is still a partial class, allowing users to customize calls via partial methods for intercepting requests/responses. ConfigurationThe Configuration objects implement
This merge works similarly to various JavaScript framework utilities merge/extend functionality. For example, Lodash's GlobalConfiguration classA partial class exposing a Singleton object… MultimapHTTP allows multiple values to be present in a single header. OpenAPI allows query string parameters to represent a collection of items for a single query string key. There was previously no clean way to represent this. There's nothing particularly fancy about this implementation. It's essentially decorating a As with all types in the generated code, users are welcome to reimplement this type and add it to RequestOptionsAbstracting away from RestSharp means having our own implementation of request input parameters. This is implemented as API filesAll APIs generate 3 interfaces:
This allows consumers to pass the preferable implementation via interface (rather than passing the class and accidentally calling a synchronous operation where an async operation was intended). Constructors are cleaned up to make fewer assumptions. Properties allow users to get/set properties, with the assumption that consumers will use this functionality properly. The reason for this refactor is that many consumers want to use something other than RestSharp as the underlying query framework. Users can now extend in some ways which have been requested, but would previously require local modifications to generated code and ignores:
|
I'd like to. There have been discussions about deprecation process, but there's been no target for deprecation of any generators as far as I am aware. |
@wing328 I've regenerated a sample if you want to take a look at the implementation. |
@mandrean I was wondering if you'd have any feedback on this refactoring approach? |
@jimschubert, I've just looked at the template, I notice there are errors during CI, are these planned to be corrected? |
@SeanFarrow the errors in CI are because tests for all C# clients are strongly reliant on the old RestSharp specific API. I've only regenerated a single client sample, which you can evaluate manually in this branch by following the "To evaluate" steps in the PR description. As mentioned in my second comment on this PR, I'm waiting for feedback on the new design before updating tests. This will save me time if, for example, someone is opposed to any of the designs I've implemented here. I hope that helps clarify. If not, I can provide a more detailed evaluation step. |
@wing328 any concerns with planning to make this the default template in 4.0.0? I'm hoping to have some time this weekend to resolve the test changes, and update some documentation around extending the client generated code to fit a user's needs. |
We'll instead keep the current Ref: #2348 |
* [csharp] Refactor to support third-party customization more easily * [csharp] Regenerate OpenAPIClient sample * create new csharp-refactor client gen * update samples * fix Locale.ROOT * fix import * remove outdated files, update samples
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
and./bin/security/{LANG}-petstore.sh
if 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\
.master
,4.0.x
. Default:master
.Description of the PR
A refactoring proof of concept for the C# client.
This puts a client abstraction on top of RestSharp, such that consumers can provide a custom implementation. This is useful for SDK creators who would rather use HttpClient, or consumers who would rather construct APIs with their own solution (e.g. in-house api accessors with tracing and logging applied).
This PR includes only template changes, as generating working C# clients is currently broken on master due to a regression in enum processing (see gist).I'm opening this for feedback.
To evaluate:
cd samples/client/petstore/csharp-refactor/OpenAPIClient sh build.sh csharp -r:bin/Org.OpenAPITools.dll -r:bin/Newtonsoft.Json.dll -r:bin/RestSharp.dll