Revise comments, docstrings, some messages, and a bit of code by EliahKagan · Pull Request #1725 · gitpython-developers/GitPython (original) (raw)

This revises comments and docstrings, to improve clarity, readability, stylistic consistency, in some cases accuracy, and (for docstrings) Sphinx formatting and cross-linking. It also contains a few other related changes.

Short summary of the most notable changes

In both comments and docstrings, my changes included rewording for clarity, as well as changes in capitalization, punctuation, and so forth.

For comments, which were in a variety of styles, I converted them to be sentences (or in some cases fragments formatted as sentences), except in a minority of cases when doing so seemed like it would worsen readability. I did this because, in addition to improving stylistic consistency and being encouraged by PEP-8, this tended to make their structure clearer (especially in cases of comments over multiple lines that were convertible to multiple separate sentences). This comment style was also one of the styles already represented throughout the codebase.

For docstrings, there were a number of places where the formatting looked one way when reading the raw docstring, but very different, and arguably less readable, as rendered by Sphinx. In cases where this was clearly unintended and the effect was significant, such as markup that was intended to represent an unordered list but instead represented a running paragraph with literal asterisks, I fixed the formatting. In cases where the intent was less clear, such as non-wrapping newlines conveying a break more major than that between sentences but less major than that between paragraphs, I preserved the style where possible but did not make changes to cause it to be newly reflected in rendered documentation.

Kinds of other changes

There are also some other changes, described below, including some code changes, mainly for clarity, that I believe to be sufficiently related or where there is otherwise a reason to include them here rather than proposing them separately.

Some specific changes (not to code) are described in commit messages but not this PR description, but this is noted below. Specifically, a handful of changes to docstrings are for accuracy, and to allow them to carry specific rationales and to be reviewed individually, they each have individual commits and are not currently relisted here.

Line length in docstrings

I found that many docstrings were already written, or almost entirely written, with a hard wrap of 88 columns. It makes sense that this would be done, because it is the black default, even though black does not enforce line length in docstrings, and furthermore the maximum line length this this project has been set at the much higher 120 columns. I think that a width of 120, or even 100, in docstrings, reduces readability when reading the docstrings in the code (of course it does not affect what Sphinx renders), so this is probably another reason a smaller width was used in most cases.

Where width was consistent, as well as where making it consistent didn't seem like it would improve readability, I left it alone. Otherwise, I prioritized consistency within each docstring and with nearby docstrings, but overall (when making changes that would justify it) preferred the prevailing 88 column width. To a much lesser extent, I preferred a width of 88 columns for comments, but only where that style was otherwise being used. I did not format code to make it narrower. We may want to reduce the black maximum line length from 120 to 100 or even the default of 88, but I definitely would consider that beyond the scope of this PR.

Documentation generated from tests

This includes changes to comments in portions of the two test modules from which documentation is generated. This is not special, but I wanted to note it here to properly characterize what parts of the generated documentation are and are not affected.

Other documentation - only one small change

Although most of the the docstring changes affect generated documentation, and the documentation in doc/ should also be revised and updated (and in some places stands to benefit more from such changes than anything here), the purpose of this pull request is not to revise or update the separate documentation in doc/.

This does include one change to the contents of doc/, to avoid introducing an inconsistency when rewording a docstring for clarity whose wording was also used in the tutorial. Other that that and a change in conf.py, it does not modify any tracked files in doc/.

How the changes are divided into commits

Formatting, rewording, and style changes such as to capitalization and punctuation, as well as the repair and introduction of Sphinx cross-linking changes and code-formatting changes (such as placing every multi-paragraph docstring's closing """ on its own line) are done in a small number of commits, most pertaining to organizational sections of the code.

In contrast, accuracy improvements, even though they account for a much smaller volume of change, are each done in their own commit, so they can have commit messages that explain their rationale and to make them easier to review.

Although I had considered describing each of those changes here, I have not thought of a way to do so that I think would clearly be more useful than the commit messages themselves. Furthermore, this description is already rather long. So I have not reproduced that information here. However, I would be pleased to expand this PR description on request, either about that or in other ways.

What is omitted

In areas where accuracy might be improved but it is unclear how, or where reviewing the changes seems like it would be easier done separately, or where the changes should be accompanied by code changes that themselves are not natural to include here, I have omitted such changes entirely from this pull request. (One such area I discovered while working on this but did not include in it was #1712, which I instead fixed in #1714.)

The changes in this PR are probably incomplete, in the sense that I most likely have missed some things.

In some areas, I have avoided making changes that I anticipate would create conflicts, or other confusion, with work I have on other feature branches that I plan to open PRs for soon. In practice, the only major omission that arises due to this is in test/test_util.py, which is mostly unrevised since I intend to propose changes there soon, for making xfail markings more precise, that will involve reorganizing that code.

Changes to code

Finally, please note that this PR does contain some code changes. They are as follows:

• A few messages, such as exception messages, are revised. I avoided doing this except when I thought it was a clear improvement and would not cause problems. When accuracy was the motivation, such a change has its own commit (as do other accuracy changes).
• Some string literals are changed, so that the use of raw string literals is consistent (for regular expressions and for Windows paths not ending in a backslash, and with r and R respectively). Commit messages contain further information about this. Conceptually, I felt this came along with fixing docstring bugs where '\' was intended to represent itself literally but instead represented '' because a non-raw docstring turned \' into '. There is also an argument to be made that strings are the core concept of this pull request (hence the branch name).
• Explicit inheritance from object is removed. This is a legacy of Python 2, where it was needed to make a new-style class. This project no longer supports (and is already considerably incompatible with) Python 2, and in Python 3 all classes are new-style classes. The reason this is a clarity improvement sufficiently related to documentation that it seemed appropriate to include here is that GitPython has an Object class that could be confused with object.
• Expressions of the form x.__class__ == t are changed to x.__class__ is t. In #1673, I fixed the corresponding problem for type(x) == t, but flake8 does not detect the .__class__ variant. The reason this change is convenient here is that most of these changes were on the same or an adjacent line to something I was already changing, so making them separately would in most cases have incurred a conflict. But the reason I think it's reasonable to include this here is that a number of GitPython's objects have type attributes that pertain to Git and GitPython's data model rather than to the Python object model, and that hold strings that are (correctly) compared with ==. Comparing x.__class__ with is, as the related type(x) is compared but as the unrelated x.type is not compared, makes the distinction easier to heed, I think.
• Python permits __slots__ = "slot" for a single slot slot, but it is discouraged in preference for __slots__ = ("slot",), which was already being done in most cases. I fixed the few occurrences of the former to bring them in line with that.

Although readability would be further improved by replacing two-argument super calls with zero-argument super()--which is also something Python 3 adds that is related to new-style classes and thus arguably beckoned by those last three bullet points--I have omitted this. The reason is that this change would require special attention in review to ensure that it is only done in cases where the meaning of the code is not changed. (That is likely every occurrence of super in the whole repository, but they would still have to be checked individually.)