Code review comment for lp:~allenap/gwacl/list-files-by-prefix

Revision history for this message
Gavin Panella (allenap) wrote :

> (You gotta wonder why Go doesn't have default args :/)
>
> The first thing that popped into my head was, do you think that we should put
> mandatory parameters as separate args to the ListAllBlobs function, and
> reserve ListBlobsRequest for optional params only?  It seems as though you
> don't check for the mandatory params anyway, but it would obviate that param
> checking and is more explicit.

Now that we have a struct to represent the request, I think it should
be the place that it's represented. We could add a Validate() method
to the struct to check always-required parameters or something, but
I'm more inclined to just throw at Azure whatever is passed in, and
let Azure do the checking. Let's make the fewest assumptions about
Azure possible. It's less work too.

>
> Second thing: the tests nearly all have the same set-up code for the fake
> transport.  I think we need a factored test helper for this as all the setup
> detracts from the test code's intention.  I don't think it should be done in
> this branch though, a follow-up one is better.

Yeah, it's ugly. We can extract some of that stuff, but it may come
with a loss of clarity. I've seen enough of this code for now. Given
time I may come back to it and see a good way to clean this up; the
same goes for any of us.

>
> Also - I am wondering if we need some factory methods to create all these huge
> blobs of XML we're passing around? It's not entirely clear what we need
> though.

Yeah :-/ Same goes as for the set-up.

>
> Everything else looks pretty good to me, thanks for all the extra test cases!

Thanks for the review!

« Back to merge proposal