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

Refactor to use async methods for network calls #30

Closed
wants to merge 2 commits into from
Closed

Refactor to use async methods for network calls #30

wants to merge 2 commits into from

Conversation

steverichey
Copy link

@steverichey steverichey commented Apr 28, 2017

This change makes all of the public methods in ILRS return Task objects, and implements these as async methods in RemoteLRS. Some related changes:

  • MyHTTPRequest and MyHTTPResponse have been moved to LRSHttpRequest and LRSHttpResponse
  • HttpClient is used instead of HttpWebRequest for better async support, and so that this library can be used as a PCL, as HttpWebRequest is not part of the .NET Portable Subset.
  • The .NET Framework version has been updated to 4.5 (async only requires 4.0, but HttpClient requires 4.5).
  • NUnit has been updated to 3.6.1 to support async tests.
  • xAPI v1.0.3 has been added to TCAPIVersion to allow unit tests to pass.

This is the first in a series of pull requests, as we made a number of changes to this library at @gowithfloat for a recent project, and would like to integrate them back into the core library. Some of these changes will be breaking (like this one).

I know there's another pull request that adds async version of methods, but as these are all network calls, they should be refactored to only be async. There's also a related issue currently open.

Please let me know if you have any questions or comments. Thanks!

required for current integration tests to pass
this requires .NET Framework v4.5 to use HttpClient, and an update to NUnit 3.6.1 to support async tests. this should also allow this library to function as a PCL, as HttpClient is available while WebRequest was not.
@OliIntraz
Copy link

I am looking into using the TinCan.NET library in a .NET Core web application and this should do the trick, any news on when/if this pull request will be merged and published?
Good job, btw:)

@steverichey
Copy link
Author

Is there anything I can do to get this merged? We have a number of other changes we'd like to see included in the official distribution of this project. Any feedback from @brianjmiller or any other interested parties is highly welcome.

@srijken
Copy link

srijken commented Apr 17, 2018

+1. I'm also looking at .NET Core, async/await and attachment support. I'm willing to do PRs, but they have to be looked at and accepted when OK. This PR, and also #18 don't give that impression

@reedmclean
Copy link
Contributor

@steverichey, really sorry for the delay on this. We've had some trouble finding the time to tend to our OSS repos, but the plan is that I will be able to start taking on some of the work from here on.

Everything you've got here looks great! The only concern I've got is that, like you said, this is a breaking change due to both the changes to ILRS and the target framework bump, so we'll need to push this into a new major version.

I'm also seeng a lot of interest in expanding coverage to .NET Core (like @srijken mentioned), so I'm working on cobbling together of a list of changes that we want included in the next major version. I'm also going to figure out if there any non-breaking changes that we should include before bundling up the next major version. I'm going to spend a bit of time catching up and getting that info together, and I'll get back to you when I've got a rough roadmap.

Thanks!

@reedmclean reedmclean self-assigned this Jul 30, 2018
@reedmclean reedmclean mentioned this pull request Aug 9, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants