(Super skeptical of an AI review but I'm curious, so...)

seems simple enough, and btw AI is good enough to find issues (I use it near daily), but very far away from replacing us :P.

@mholt i would add omitempty and i think this is good.

i would add omitempty and i think this is good.

Thanks; I think only ProviderData (as currently named) would ever be empty... I think? Maybe TTL, depending on when/where provider defaults are applied.

I also added JSON tags to the RR struct, in preparation for 1.0, since JSON-serialization seems like it could be a common pattern.

👍

Finally, I am torn between Meta, Metadata, and ProviderData for the field name. Any thoughts?

ProviderData seems like the least-worst name that I can think of. It's not an amazing name, but certainly better than anything else that I can think of.

However, since provider implementations are supposed to return the concrete struct types, not RRs, we'll likely need to add this field to all the specific structs (Address, TXT, CNAME, etc...).

Hmm, yeah, you're probably right here. So then calling .Parse() or .RR() should probably just copy over the contents of ProviderData unmodified? I guess the other options would be to provide a hook for this or unconditionally set ProviderData to nil, but neither of those seem great to me.

Or, maybe it would be better to only have ProviderData on the specific structs, and not on RR? My thinking being that you can change the Type of an RR, and providers might handle that very poorly. I don't think that this should be very much of a problem though, just something to think about.

@gucci-on-fleek What do you think?

The main problem that I can see is that we'll either need to document that RRs aren't comparable

which might be annoying since that means that there's no “easy” way to compare records, or document that ProviderData must be primitively-comparable, which would also be annoying since then you couldn't use it to hold a map.

The other problem that I can think of is that weird things might happen if you try and copy Records between different providers, so we should probably document either that libdns implementations must ignore invalid/unknown ProviderDatas, or that users must set ProviderData to nil when copying between zones/providers.

But overall, I'm very much in favour of this.

@gucci-on-fleek Great points, thanks!! Super helpful as usual.

I now agree that we shouldn't put this in the RR type.

It's a bit annoying, though, to put it in the individual struct types, so I just had an idea.

What about this:

type ProviderRecord struct {
	Record

	// Optional data associated with the provider serving this record.
	// See the package godoc for important details on this field.
	ProviderData any
}

This way, provider packages can return ProviderRecord values, which include custom data, and callers can still access the underlying Record independently.

Of course... this breaks callers' type-switches, which might be a deal-breaker. They'd have to specifically type-assert for this struct, then access the Record field to use it. Hmm.

Yeah, maybe just putting it on each of the individual record type structs is better. I'll work on that.

@gucci-on-fleek Okay, I reworked it so that ProviderData is in each individual struct type, not the RR struct. I also added some package-level godoc surrounding it.

Since packages are supposed to return the individual struct types anyway, they can just attach their custom data in this field, and callers' type assertions will still work. Obviously, the provider-specific data is lost with RR() (and thus, Parse() does not populate it), but that's OK since the RR type is meant to be more generic/portable, and comparable.

Awesome, thanks!