alanedwardes/Ae.Dns

Documentation is outdated

SamLowryMOI opened this issue · 2 comments

I had a look at your project. Extensive work!

Unfortunately, it took a while to get it up and running because the documentation is not up to date.

  1. DnsAnswer was probably replaced by DnsMessage.
    DnsHeader was probably replaced by DnsQueryFactory.

    So instead of
    DnsAnswer answer = await dnsClient.Query(DnsHeader.CreateQuery("google.com"));
    it must read like this:
    DnsMessage answer = await dnsClient.Query(DnsQueryFactory.CreateQuery("google.com"));

  2. The call DnsCachingClient(upstreamClient, memoryCache) is not possible, probably a logger was added.

    Therefore,
    DnsAnswer answer1 = await cacheClient.Query(DnsHeader.CreateQuery("google.com"));
    becomes
    IDnsClient cacheClient = new DnsCachingClient(logger, upstreamClient, memoryCache);.

    At this point, a sample logger would then be helpful, such as:

    using var loggerFactory = LoggerFactory.Create(builder =>
    {
        builder.AddConsole();
        // or/and 
        // builder.AddDebug();
        builder.SetMinimumLevel(LogLevel.Trace);
    });
    
    var logger = loggerFactory.CreateLogger<DnsCachingClient>();
    

    or at least

    var logger = NullLoggerFactory.Instance.CreateLogger<DnsCachingClient>();
    

    Even better (IMHO) would be to change the function:

    public DnsCachingClient(IDnsClient dnsClient, ObjectCache objectCache, ILogger<DnsCachingClient>? logger = null)
    {
        _logger = logger ?? NullLoggerFactory.Instance.CreateLogger<DnsCachingClient>();
        _dnsClient = dnsClient;
        _objectCache = objectCache;
    }
    
  3. Note
    Sometimes using is used, sometimes not. Shouldn't it better always read:
    using IDnsClient dnsClient = [...]

So far I've only tried the client, it may look similar in the further documentation.

Thanks for highlighting this - while I like putting code in READMEs, API changes break it and it's difficult to notice. I think I am going to move the readme samples over to separate console app projects, so that they're built with the rest of the solution and it's clear that they work.

The other side of this is breaking changes in general - the library is 0.x for a reason, though now I think it could be at version 1.0.0 as I'm happy with the API as it stands. I will look to release a 1.0.0 in the next few weeks since it puts a line in the sand for this type of breaking change.

I'm happy with the API as it stands.

You really can be, good job!
I hope you understand this as it is meant, as feedback and support.

There is still DnsAnswer mentioned, which I think no longer exists.

Also see the following types:

    DnsHeader
    DnsAnswer
    DnsResourceRecord
    DnsIpAddressResource
    DnsMxResource
    DnsSoaResource
    DnsTextResource
    DnsUnknownResource