Added POC arbitrary query strings

[oholiab]

So there's a strong chance that this particular incarnation of this PR will be rejected, but I figured a POC would be better than trying to say what I mean in an issue

I've modified the ListWAFRules function to take an optional url.Values... My personal use case for this being that I want to see ALL my WAF rules, so ?per_page=1000 for example.

Obviously this is introducing a special case, so a couple of ideas would be:

• Do this on *, or at least all places it makes sense (messy but would make everything super flexible)
• Add a url.Values to the API struct that can be used as a default, and add it on to the URL within makeRequest (much tidier but modifying would no longer be concurrency safe)
• Both (which I quite like, as you can have your default set in the API struct and any special rules explicitly entered per request)

Equally, I'd be very happy for this to be accepted in to master as it's on my critical path, but beyond that the second option would help immensely for my use case.

Anyway, let me know what you think, I'm happy to do some more leg work on this.

[elithrar]

Both (which I quite like, as you can have your default set in the API struct and any special rules explicitly entered per request)

This gets my vote—some of the defaults (e.g. 20 per page, 50 per page) are quite low and it's preferable to save another HTTP round-trip where possible. You could implement this as a method on API with a warning to not call concurrently/during requests (i.e. api.SetPerPage(n int)).

The downside to that approach is that some values may not make sense for every endpoint, and passing a nil parameter is not the nicest API - e.g. ListWAFRules(zoneID, packageID, nil).

[oholiab]

If we stick with the pattern I've demonstrated in ListWAFRules for the other methods, because it's variadic you don't actually have to pass anything as the third argument. If we make API.makeRequest take the same (that is 0 or more url.Values) it would be more DRY as we can either implement validation (i.e. error on more than one instance passed in) or even simple merging within the API object.

[jamesog]

I'm fine with this PR as-is, but I'm definitely open to making this more DRY, as I'm already doing similar things in dns.go and zone.go (and I'm not entirely happy with the way I'm doing it there).

I like the idea of extending the API struct and having a method call to modify it.

@oholiab if you like I'll merge this and we can do the other work later, or if you've already got something in mind feel free to push it to this PR for review.

Thanks for contributing!

[elithrar]

because it's variadic you don't actually have to pass anything as the third argument.

As long as we merge multiple url.Values into a single map (or error out if they provide > 1) that'd work. You & I (and James) all know what is intended here by the variadic arg, but other users might pass three (3) distinct url.Values instances.

Added: another option is functional options - e.g.

+func (api *API) ListWAFRules(zoneID string, packageID string, opts ...APIOption) ([]WAFRule, error) {

...

type option struct {
    perPage int
    ascending bool
    etc.
}

type APIOption func(option) option

func PerPage(n int) APIOption {
    return func(o option) option {
         return o.perPage = n
    }
}   

// Example
api.ListWAFRules("1173813", "200318138", cloudflare.PerPage(100), cloudflare.Ascending())

[oholiab]

I think maybe hold off on merging actually - I should be able to knock
together the extension to API and associated methods on Monday and then
after that's merged, rebase this branch on master and attempt to glue the
two ends together at a later date - that way we won't have any regression
in master between commits and I can get on with my terraform code without
having to worry about compatibility.

Sound good?
On 11 Mar 2016 18:53, "Matt Silverlock" notifications@github.com wrote:

because it's variadic you don't actually have to pass anything as the
third argument.

As long as we merge multiple url.Values into a single map (or error out
if they provide > 1) that'd work. You & I (and James) all know what is
intended here by the variadic arg, but other users might pass three (3)
distinct url.Values instances.

—
Reply to this email directly or view it on GitHub
#4 (comment).

[elithrar]

That works too. Appreciate the work/effort @oholiab!

[jamesog]

Sounds good to me.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.