I like this PR overall; the only thing I went looking for is .. if it's somehow possible to make the zero value "work", i.e., something like this will panic;

type Something struct{
	Filters Filters
}

foo := Something{}

foo.Filters.Add("hello", "world")

Because in general, I'd like to get rid of the bare filters Filters as arguments, and always use "some option structure" for these arguments.

Changing the methods to be on a pointer-receiver ... would allow fixing that, but not great either.

And I guess making it back into a struct with the map as a field would bring us back to the type not working to be included in a struct (for JSON requests)

Happy to hear if there's options to ... somehow fix that though!

I had initially written it to make the zero value writable, but ultimately backed out. It would break with Go convention for map types to have methods that initialize the map as a side effect. And I ran into some weirdness with method chaining when I tried that, IIRC.

What if we put convenience methods on the options struct?

type Something struct {
        Filters Filters
}

func (s *Something) AddFilter(key string, values string...) *Something {
        if s.Filters == nil {
                s.Filters = Filters{}
        }
        s.Filters.Add(key, values...)
        return s
}

Related to the above; do we need to make a mention of it panicking if Filter is nil? The issue with nice methods (like Add) makes it easier to assume "it just works" (whereas dealing with a raw map makes it more obvious "this must be initialised before use!"

I called that out in the documentation for the type.

// Like all other map types in Go, the zero value is empty and read-only.

The Go authors did not feel it necessary to document that ("net/url".Values).Add() panics on a nil receiver. It's the expected convention that nil-valued map types are not writable.

That one definitely tripped me up a few times though 😂

Add not only adds, but replaces what was already there; or at least for the term key, it's, it would keep it as-is, but for values, it replaces what was there.

Wondering if Set would be a more appropriate name? Alternatively AddValues I guess could be an option.

No, Add appends, same as (url.Values).Add. I'll cover that in the unit test.

DOH! You're so right! It's not a key/value pair; the boolean is just because it needed a value. I should think before I post; was even thinking "this could've been a struct{}{}" (although those are awkward for the API).

I've consider a few times that it probably should've kept the "old" format; just a slice, and handle de-duplicating (if needed) when creating and on the daemon-side, instead of that optimisation making its way to the API format, well 🤷