Code review comment for lp:~julian-edwards/gwacl/pageblob

On 18/06/13 18:06, Jeroen T. Vermeulen wrote:
> The capitalization of PutBlobRequest looks suspicious: the type is exported but its attributes are not. AFAICS its only use is in PutBlob, but how is anybody outside the module going to be able to create a PutBlobRequest to pass to PutBlob if they can't address its fields?

I forgot about the capitalisation exporting crap. Bugger. Why.... why
does it have to be like that.

>
> Is there any value to passing the address of a PutBlobRequest into PutBlob, as opposed to just giving it the struct itself? It just seems like extra weird syntax to live with: &PutBlockRequest{...} instead of just PutBlockRequest{...}. Then again, it's probably consistent with what we did in other places...

Exactly. I considered passing by value but it would be inconsistent,
and consistency always wins.

>
> By the way, blob_type in Go style becomes blobType, and extra_headers becomes extraHeaders. Not pleasant, especially when it goes against vocal emphasis, but we'd better get used to it.

Grrrrrr!

>
> Once inside PutBlob, you return an error when an invalid blob type is selected. But this isn't variable data, so I'd say this was indicative of a static programming mistake. Might as well panic to teach them a lesson. And if that seems unfair, well, document the thing so they get a chance to do better!

Fair enough!

>
> The comment on TestPutBlob, about PutBlob returning Created on success, no longer belongs on top... It's now one of many things that the test looks at.

Changed.

>
> On TestPutPageBlob, the documenting comment incongruously starts with "Ensure that." Might as well drop that and just state the behaviour that the test verifies as fact.
>

Ok.

Better now? :)