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

Added async versions of network functions #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

limey98
Copy link

@limey98 limey98 commented Dec 23, 2015

Also, updated to .NET 4.5.

Sorry it took 2 additional useless commits, I'm relatively new to Git (and GitHub) and was screwed over by line endings.

I did consider adding compilation symbols for .NET versions (from http://stackoverflow.com/a/29001688) and wrapping everything in #if statements, but it seemed like more effort than it was worth.

Requested in issue #13, but also something I think is pretty much required in this day and age.

@scara
Copy link

scara commented Dec 23, 2015

Hi @limey98,
you can "squash" the extra commits by using the interactive rebase: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History#Changing-Multiple-Commit-Messages.

HTH,
Matteo

@limey98
Copy link
Author

limey98 commented Dec 28, 2015

Thanks @scara. Took a while, but I got it sorted.

@musicm122
Copy link

Is this going to be merged or is the issue still opened?

@brianjmiller
Copy link
Member

Async functionality has not been added. This PR has conflicts with the master branch that need to be resolved, probably by rebasing. Additionally there are changes in the PR that need to be avoided and only those strictly needed for async capability should be in included. For instance bumping versions of dependencies and/or the .NET framework version should not be included, except where necessary to achieve the functionality and if those are required they will necessitate a major version bump which gets increased scrutiny. Sorry for the delay on the review, it got lost in my inbox.

@limey98
Copy link
Author

limey98 commented Oct 11, 2016

I can try and sort out those conflicts at some point later today or tomorrow. As for the .NET Framework increase, it is unfortunately required for the proper async stuff. Is that completely impossible? If so, I can rework it to use the older, crappier version of async in .NET that barely deserves the name.

@brianjmiller
Copy link
Member

I don't think it is necessarily impossible to update the framework version, it just is in the 1.x.x.x release cycle if we're going to maintain semver. Which isn't necessarily a problem, though I am curious if there are any significant systems (like Unity) that will break completely by us doing that. Generally I just want to avoid making changes because Visual Studio likes to bump things, etc. but if there is good reason (like this one) then it is fine.

@limey98
Copy link
Author

limey98 commented Oct 11, 2016

Funnily enough, in my day job (a Unity developer at an e-learning company), I've modified this library to work with it. The async stuff I've done here doesn't work (Unity still uses .NET 2.0), but I don't think this library works out the box either. I added callback versions of all the functions, made them asynchronous with StartCoroutine and used Unity's inbuilt networking (both new and old) to send the actual request. I'll try and get around to re-implementing that in my own time and doing another pull request with that.

@limey98
Copy link
Author

limey98 commented Oct 12, 2016

I've rebased my changes.

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.

4 participants