Clarify Git.execute and Popen arguments by EliahKagan · Pull Request #1688 · gitpython-developers/GitPython (original) (raw)

This is a sequel to #1687, improving how parameters to Git.execute are documented, and improving debug logging and tests of how they affect the arguments passed to Popen.

In the Git.execute docstring, the method is no longer described as using a shell, since this is typically (and by default) not the case; the items documenting each parameter are listed in the order of the parameters; and some copyediting is done, for clarity, consistency, spacing, and in a few cases other formatting. I copyedited some other docstrings in the module as well.

The order of names in the execute_kwargs set is tweaked to also match the order in which they appear as parameters to Git.execute. (This is just for the purpose of code clarity, as set objects guarantee no particular iteration order.) Since not all unintentional mismatches between this set and the defined method parameters would cause existing tests to fail, and the failures that would occur would not always immediately show the cause of the problem, I also added a test that execute_kwargs has the exact expected relationship to the parameters of Git.execute. (Though an alternative might be to generate execute_kwargs programmatically from Git.execute using a similar technique.)

One part of the test logic in #1687 was unnecessarily complicated, due to swallowing an exception produced by running git with no arguments. This changes that by passing with_exceptions=False so that exception is never generated.

Another part of the the test logic in #1687 combined claims about the code under test with custom assertion logic in a way that made it hard to see what claims were being made by reading the test code. This fixes that by generalizing, and extracting out, an _assert_logged_for_popen test helper method. I also took this opportunity to remove its unstated incompatibility with value representations containing regular expression metacharacters. (Please note that the new code is still only robust enough for the special purpose for which it is intended; it does not actually parse the debug message rigorously.)

As requested in #1686 (comment), I changed istream= to stdin= in the debug logging message documenting the Popen call. I reused the test helper in writing a new test, test_it_logs_istream_summary_for_stdin,
which checks both that it has the new name and that it has the expected simplified value representation. I did not change any parameters or variables called istream to stdin or any other name; rather, this changes the message to better reflect the Popen call it exists to document.

If I would only have changed the displayed parameter name, I would likely not have written a test, but I also wanted to change two other things, in which the test helped verify that correctness was maintained: I reordered the displayed name=value representations in the log message to match the relative order in which they are passed to Popen. And I eliminated the istream_ok variable (which was named like a boolean flag but was not one), because having the logic for producing the string in the logging call makes it better resemble the nonidentical but corresponding logic in the Popen call, so they can be compared.

Although this is peripheral to the core concept of the clarity of function arguments, I also renamed and fixed the docstring of a local function that appeared (including by claiming) to be a method.