search

robertvazan / guerrillantp

Simple NTP (SNTP) client library providing .NET applications with accurate network time.
https://guerrillantp.machinezoo.com/
Apache License 2.0
67 stars 17 forks source link

Default Constructor and Async support #6

Closed TonyValenti closed 2 years ago

TonyValenti commented 2 years ago

Would you accept a PR that adds:

  1. a default constructor (pool.ntp.org)
  2. Async methods
  3. A "GetNowAsync" method

Thanks!

robertvazan commented 2 years ago

Sure. This will probably require upgrade to .NET 5, which I am in favor of.

TonyValenti commented 2 years ago

Hi @robertvazan - I'm working on this code and there are some other minor adjustments I'd like to make and I wanted to get your permission first. Can I do the following? (yes/no for each item)

  1. Can I use #if to make the library build for .NET Standard with no async support and .NET 6.0 with AsyncSupport?
  2. Can I create a static NtpClient.Default instance that points to ``pool.ntp.org```?
  3. Because the most common use of GetCorrectionOffset is to add the value to DateTime(Offset).(Utc)Now, can I cache the last GetCorrectionOffset result and add DateTime(Offset)(Utc)Now properties that will do that math?
  4. Can I update the version number to be 6.0.0 to align with .NET 6.0?
  5. Can I rename Query to GetPacket / GetPacketAsync?
  6. Can I remove the need for NtpClient to implement IDisposable by moving the socket code into GetPacketAsync?
robertvazan commented 2 years ago

@TonyValenti Thank you for your interest. As for your questions:

  1. Would async work on .NET 5 too? If so, it might be simpler to just upgrade to .NET 5. I don't believe anyone still targets platforms that require .NET Standard. Last version compatible with .NET Standard can be mentioned in the tutorial.
  2. Sure. Is DNS lookup going to be done in the constructor? It would be better to perform it in every query to avoid blocking in the constructor. Is there async API for DNS in .NET 5?
  3. The API must offer an easy way to differentiate between blocking/async methods/properties and their cached versions. I would be more in favor of putting the new properties in NtpPacket and having two methods on NtpClient, one waiting for new NtpPacket (i.e. current Query and its async version) and one returning cached NtpPacket (perhaps Last property?). GetCorrectionOffset should be deprecated in favor of CorrectionOffset on NtpPacket.
  4. This project uses semantic versioning. I will set appropriate version when I make a release.
  5. Why break existing user code? GetPacket misleadingly suggests latency-free getter.
  6. Sure. No need to keep the socket open.
robertvazan commented 2 years ago

PR was merged and I have already added my changes too. Closing as done.

RobThree commented 1 year ago

I don't believe anyone still targets platforms that require .NET Standard

It is what Microsoft (sort-of, half-heartedly) recommends for libraries though:

Reusable libraries

If you're building reusable libraries that you plan to ship on NuGet, consider the trade-off between reach and available feature set. .NET Standard 2.0 is the latest version that's supported by .NET Framework, so it gives good reach with a fairly large feature set. We don't recommend targeting .NET Standard 1.x, as you'd limit the available feature set for a minimal increase in reach.

If you don't need to support .NET Framework, you could go with .NET Standard 2.1 or .NET 5/6. We recommend you skip .NET Standard 2.1 and go straight to .NET 6. Most widely used libraries will multi-target for both .NET Standard 2.0 and .NET 5+. Supporting .NET Standard 2.0 gives you the most reach, while supporting .NET 5+ ensures you can leverage the latest platform features for customers that are already on .NET 5+.

Personally I target netstandard20 (mostly because it provides compat with .Net framework 4.6.1/2 and up) unless net60 or higher provides substantial benefits. But that's me and this is, naturally, all up to you. I just wanted to point out the MS 'advice'.

robertvazan commented 1 year ago

@RobThree Thanks. I have opted for .NET 5+ while still linking to the last version on NuGet that supports .NET Standard. Much cheaper and almost as good as multi-targeting.

RobThree commented 1 year ago

Much cheaper and almost as good as multi-targeting.

How about when you get it for free? 😉 See #12

I was JUST about to do the PR before you replied.

Anyway, that PR should solve #6, #10 and #11.

Edit: Sorry, my bad, not #9 but #8 (partially)