Code review comment for ~racb/git-ubuntu:push-for-upload

Thank you for the review! As discussed, there are a bunch of open questions that I'd like leave open for now, landing the branch and releasing a beta, to get user feedback. This includes some of your review points, which I agree should be addressed before a final release.

> Is there a reason this couldn't just be simply 'git ubuntu push'?

> I think that would be both simpler and more intuitive (I was actually a bit uncertain if 'push-for-upload' implied it also ran dput, but 'git ubuntu push' would feel more analogous to 'git push').

I thought "git ubuntu push" would be assumed to be analogous to "git push", and therefore its output of "strange" command line parameters (which is its real purpose) would be confusing to new git-ubuntu users. So I didn't choose "push" as the subcommand name. I'm not sure I like "push-for-upload" either; I chose it for now because it also carries the implication that it's doing something different because you want to upload (as opposed to pushing for review, for example), and I was hoping "for" would imply that there wouldn't be additional side-effects such as a dput.

I'd like to land this as "push-for-upload" for now, but leave the subcommand name as part of the experiment and settle the final name further down the road.

> Note the code docs in the tests use single quotes, while the one in push_for_upload.py has double quotes.

I looked up PEP 8 and PEP 257, and they require double quotes. So let's make that our convention across the board. I won't bother updating code not touched by this MP, but from here on, double quotes it is for any new docstrings. I've updated this branch accordingly.

I would add to doc/STYLE.md except it already says "use PEP 8" and PEP 8 tells us to use double quotes, so it's already included by reference.

> Can you also add an OUTPUT_FORMAT setting for debuild; I think that command just wrappers dpkg-buildpackage so probably uses the same values. (Maybe it could just be an alias.)

Is it really worth doing this? Adding an alias needs more (alias-handling) code, dpkg-buildpackage (ie. debuild) is the default so nobody would need to change it for debuild anyway, and in any case debuild takes "dpkg-buildpackage compatible parameters" so if you consider the meaning of --output-format to be the output _format_ rather than the destination, it's correct to say --output-format=dpkg-buildpackage and pass the output to debuild anyway.

> For --output-format the help text is just "Output format", which feels like it could be made more helpful. Could this be modified to indicate that the only allowed values for this are 'dpkg-buildpackage' or 'sbuild' (and/or 'debuild' if you accept by previous suggestion)?

The only allowed values are already generated by the --help output:

    usage: git-ubuntu push-for-upload [-h] [-v] [--remote REMOTE] [--branch BRANCH]
                                      [--output-format {dpkg-buildpackage,sbuild}]

I'd prefer to stick to argparse convention here, and avoid overloading that into the help parameter as well. I do intend to improve the documentation generally (manpage, describe the arguments better, fix the default output of --help) but I'd like to defer that to the end of the experiment when we know what CLI we actually want to keep.

> The command's description is set as "Push a branch for subsequent upload with dput" but this feels a bit awkward and potentially confusing for new users. The description for main() feels clearer:

> "Push to a remote and return suitable rich history reference headers"

> I know we're limited on line length, so here's a shot at saying that more briefly:

> "Push branch and print history-enriched headers for dput"

Same answer here - it's a good point, but I'd prefer to defer these bits to the end of the experiment.

> Whitespace indentation is off for first entry in the expected_result set for all three test cases.

This is deliberate. The first line is indented as normal; the subsequent lines are indented one further level to demonstrate that they are continuations of the same string.

> Would it be difficult to add a test_main_without_lpuser() (or extend test_main_without_remote()) for the situation where neither lpuser nor a remote are specified, that the appropriate error message is generated?

lpuser is prompted and set on `git ubuntu clone` if it is not set already, so it's quite a rare edge case that would lead to this I hope. I agree in principle we should define the behaviour if it is still unset, and test for that behavior, but as it's a rare case I'd like to defer it to get the beta CLI into developers' hands for feedback first.

> The test cases are checking only for the expected results for dpkg-buildpackage style OUTPUT_FORMATS. Would it make sense to check for the sbuild results as well?

I'm unit testing the Pythonic main() function which accepts a format string with OUTPUT_FORMATS already looked up. I added a test that varies output_format and ensures the output varies accordingly.

> Maybe the args to the target.main() should be passed in via @pytest.mark.parametrize, and possibly expected_results too?

I gave this a go, but I think the result is less readable. My attempt is at
https://pastebin.ubuntu.com/p/mbKwJH9KcN/. I didn't proceed as far as writing
comments and docstrings.

> I haven't actually run the command or tests yet, but hopefully the above gives some useful food for thought.

Appreciated - thanks!