Skip to content
This repository has been archived by the owner on Apr 24, 2024. It is now read-only.

Target netstandard 1.3 #51

Merged
merged 22 commits into from
Jul 4, 2017
Merged

Target netstandard 1.3 #51

merged 22 commits into from
Jul 4, 2017

Conversation

spewu
Copy link
Contributor

@spewu spewu commented Jun 16, 2017

Hi guys

Here is a pull request for changing the library to target netstandard 1.3.

As a part of doing this update I made a breaking change: All the public methods on the Client are now async. I realise you might not want this, so let me know if I should change them back to be synchronous again.

Thanks!

spewu added 20 commits June 15, 2017 12:03
Upgrade Newtonsoft.Json reference to v. 9.0.1 (lowest version with netstandard support)
Add reference to System.Threading.Thread
…quest, as that is not supported on netstandard

Change the MakeRequest method to be async because HttpClient is async
We cannot go lower than 1.3 as we need support for multithreading
@spewu
Copy link
Contributor Author

spewu commented Jun 20, 2017

Now changed the API back to synchronous to avoid breaking changes

@@ -71,7 +65,7 @@ internal class AsyncFlushHandler : IFlushHandler
_flushingThread.Start();
}

public void Process(BaseAction action)
public async Task Process(BaseAction action)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be left void?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on which layer in the application you want to switch from async to sync. In my pull request I suggest to do that switch in the topmost layer (the client). That makes is easy to add async overloads of the client methods if you want that in the future (which I think would be a good idea).

However, we can also choose to do that in the lower layer, e.g. where we are making the http requests. That means we can keep the rest of the layers without modification.

I don't see the harm in changing this method to async, as it is an internal class - so I don't consider this a breaking change.

In any case, the decision is yours. Let me know if you need me to modify it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot this was internal. This looks good!

@f2prateek f2prateek merged commit 6660ad5 into segmentio:master Jul 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants