Closed damienpontifex closed 7 years ago
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
:memo: Please visit https://cla.developers.google.com/ to sign.
Once you've signed, please reply here (e.g. I signed it!
) and we'll verify. Thanks.
I signed it!
CLAs look good, thanks!
Hi @damienpontifex Thank you for the pull request!
Please note there is some breaking changes as it changes methods
Is it possible to avoid breaking changes? Using a separate folder and file links, for example. What do you think?
Thanks!
@xVir well we could still expose the non-async methods to keep a matching interface to current and call through to the async method and use GetAwaiter().GetResult()
.
Then the async methods would be enhancements/additions to the interface for those wanting the async calls.
Is this what you are looking for?
@damienpontifex Maybe it will be better to build a separate dll for NETCore, like as it was planned in the dev branch https://github.com/api-ai/apiai-dotnet-client/tree/dev/ApiAiSDK.NETCore
Could you please take a look at it? Maybe it is possible to merge your code with the code there?
Thanks!
@xVir with this project format and setup, you could have one codebase and project and build it for .net core, .net standard, .net framework, xamarin + uwp etc. I've just pushed another commit with my suggestion so as to continue having the synchronous interface plus having the option of the async interface to use. This should mean no breaking changes so could you please take a look and happy to hear your thoughts.
I had looked at that other branch prior to starting but thought consolidating it into one code base would be advantageous vs having separate builds for each TFM? Classes such as AIDataService.cs are nearly identical across different targets in this repo so no point in having them duplicated
n.b. I also changed to use the Xunit test runner as Nunit hasn't been updated for .NET Core RTM and csproj formats. Very minor changes, but they can be run for both netstandard and .net framework with this now.
@xVir I've also noticed why the travis ci isn't running the tests due to this line in the config
- mono ./testrunner/NUnit.Runners.2.6.4/tools/nunit-console.exe ./ApiAiSDK.Tests/bin/Release/ApiAiSDK.Tests.dll
With this setup, it'd be dotnet test
.
@xVir any more feedback on this?
Hi @damienpontifex Sorry for the delay with answer. Thank you for the code! We will review it soon.
Thanks!
Resolves #6 by multi targeting netstandard and net461
Please note there is some breaking changes as it changes methods to async responses due to the nature of the HTTP request to api.ai and so if it goes forward, may also lead to a major version number increment.
I ran this against a local project by copying the source targeting netcoreapp1.1 and was mainly testing text based responses.
This may also be able to remove other WP8 and W10 targets if we are targeting correctly or can add further values in the csproj for. My exposure to these other frameworks isn't extensive, but I think the netstandard target should enable xamarin, .net core, uwp etc.