Skip to content
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

[csharp] Initial settings for v3.5 client compatibility #2132

Merged
merged 2 commits into from
Feb 18, 2016

Conversation

jimschubert
Copy link
Contributor

  • Pass -DtargetFramework=v3.5 to generate .NET 3.5 client
  • Run compile.bat or compile-mono.sh to bootstrap RestSharp and Newtonsoft.Json dependencies.

Issue #983

Opening this pull request so others can take a look at the code. Running this with optionalProjectFile=true, I'm able to open the project file and compile in Visual Studio 2015 Community. I haven't attempted to compile and run within a .NET 3.5 project.

There were a number of issues that I haven't addressed with this pull request:

  1. The SwaggerClientTest project is .NET 4.5. That project isn't compiled along with the client generator, and it would have been a lot more effort to generate that project to target both versions of the framework. The current tests are written to test the async client endpoints only.
  2. The client's project structure isn't setup to "just work" with nuget. For example, if you generate the optional project file, it doesn't find the nuget package config and integrate with Visual Studio's nuget tool window.
  3. There's no solution generated.

Those issues aren't really blockers. You can quickly generate the code, load the nuget package manager, and install RestSharp and Newtonsoft.Json (which is probably the common workflow anyway).

There were a couple places in the templates where I used the {{supportsAsync}} to determine functionality not specifically async (see compile.mustache and ApiClient.mustache). In compile.bat, I could have output the string "v3.5" and set the framework installation location conditionally, but checking for async, Task, and dynamic can all be rolled up into a single flag... I just didn't come up with a better name.

Pass -DtargetFramework=v3.5 to generate .NET 3.5 client
@wing328
Copy link
Contributor

wing328 commented Feb 15, 2016

@jimschubert RE: I just didn't come up with a better name.
I would have chosen a similar name :)

@@ -153,8 +194,6 @@ public void processOpts() {
supportingFiles.add(new SupportingFile("ApiResponse.mustache",
clientPackageDir, "ApiResponse.cs"));

supportingFiles.add(new SupportingFile("Newtonsoft.Json.dll", "bin", "Newtonsoft.Json.dll"));
supportingFiles.add(new SupportingFile("RestSharp.dll", "bin", "RestSharp.dll"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After removing these lines, the developers need to use NuGet to install these libraries, right ?

It make senses to me as we're trying to support different .net version. I'll make a note in the migration note for 2.1.5 to 2.1.6 upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, or run the compile scripts. As I mentioned in the pull request, the project isn't structured to support nuget when loading straight into Visual Studio. It's either run the compile scripts, or I can restructure the output directory to automatically restore.

Both scripts download nuget and grab the right packages based on the .net version selected. They then copy the nuget assemblies to the original bin folder, so I think the extra step is to make sure to execute the appropriate script for the platform. If I remember correctly, that's already mentioned in the readme of the generated source.

I'd personally like to generate the project file and have the nuget packages restored on build. I'd have to pay around with it to see how Xamarin and SharpDevelop handle nuget restore, if you think that'd be a better approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can recall the reason on why we provided the restsharp.dll. There's a bug with the latest version of restsharp (restsharp/RestSharp#742) which makes the file upload test failed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RE: I'd personally like to generate the project file and have the nuget packages restored on build.

I agree that's a better approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can lock restsharp version down to 105.1.0, which is what was provided previously, then it should still work as expected via nuget.

Do you think I should attempt to restructure the project output as part of this pull?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted to RestSharp 105.1.0 and made note of this limitation in the generated Readme.md.

Also, I don't seem to have the file upload test fail in either version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using mono. According to developers in that thread, .net also has the issue.

I'll merge later today and please try to restructure the project output later with another PR.

@wing328
Copy link
Contributor

wing328 commented Feb 18, 2016

@jimschubert the change looks very good to me and my test results are ok.

Thanks for the contribution to bring the C#-related generators to the next level.

wing328 added a commit that referenced this pull request Feb 18, 2016
[csharp] Initial settings for v3.5 client compatibility
@wing328 wing328 merged commit c1b4f8d into swagger-api:master Feb 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants