Code review comment for lp:~allenap/maas/cli-upload-files--bug-1187826

> Thanks.  This is something we've been hoping somebody else would do for so
> long!
>
> .
>
> The comment in test_encode_multipart_data_multiple_params made me blink a few
> times.  Maybe I just lack context.  What do you mean by "callables are called
> then used as context managers"?  Are the return values used as context
> managers?  The passive voice tends to obscure things: who calls the callables?
>
> Also, try reading that first sentence in the comment out loud — whether the
> original version or your new version.  If you got the emphasis right, I think
> that's because you already knew what the comment meant before you started.
> The way it's phrased seems to say: if you have sequences of parameters or
> files, you can pass those to encode_multipart_data() as opposed to somewhere
> else.
>
> What I think the text actually means (but without much confidence based on the
> text, which is my point) is: encode_multipart_data() accepts files and regular
> parameters in the form of sequences, so that you can pass multiple values for
> the same parameter.  But the sentence uses the passive voice *twice*, so as a
> reader I'm left with two who-does-what unknowns in the equation.  Adding
> unknowns makes equations harder to solve.

I've done what I can to improve it, largely reducing the use of the
passive voice.

>
> .
>
> I am similarly befuddled by maybe_file().  It might help if the name started
> with a verb, although the docstring suggests that that verb would be “check” —
> when clearly that's not what it does.
>
> Having said that, of course, I have probably obligated myself to come up with
> something better.  Here's a shot, with a name that I hope fits in well with
> ‘prepare_payload’:
>
>     @staticmethod
>     def prepare_parameter(name, value):
>         """Return parameter in a form usable by `build_multipart_message`.
>
>         Returns a (`name`, `value`) tuple.  For file uploads, the returned
>         `value` is a callable that returns a `file` object for the upload.
>         Otherwise, the returned `value` is simply the value that was passed
>         in.
>
>         File uploads are distinguished by an ampersand (`@`) suffix to the
>         parameter `name`::
>
>             parameter@=filename
>
>         In that case, the `value` parameter is the file's
>         path.  The returned `name` will not have the ampersand.
>         """
>
> This version also hints at _why_ a file is returned as a callable: because
> that's how build_multipart_message likes it.

I have changed how this works. It's broadly similar to what you've
written, but happens at a different point. maybe_file() is gone.

>
> .
>
> In test_files_are_included, I would suggest using factory.make_name('param')
> instead of factory.getRandomString() for generating the ‘parameter’ value.

Done.

>
> .
>
> In that same test, I'm not very comfortable with expected_body_template.  Is
> the ordering of those lines actually defined?  It'd be better if we could
> either:
>
> (a) parse the MIME message back and see that we get the expected result; or
> (b) excise the relevant parts and use Contains matchers.

Messages headers are stored in order, so this'll be fine. It'll be
interesting to see if/how this breaks as we change Python version.

Thanks for the review.