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 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.

.

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

.

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.

Jeroen