Use a docstring length of 75

[akaszynski]

Docstrings are limited 75 characters: or how I Learned to Stop Worrying and Love Python

tl;dr
75 characters enable both the readers of our online docs and those using the standard Python help on 80 character terminals to read our docs without being overly restrictive. This helps us remain as close as possible to PEP8's 72 character guidelines.

Long Explanation

First, the standard from PEP8 explains:

Maximum Line Length
Limit all lines to a maximum of 79 characters.

For flowing long blocks of text with fewer structural restrictions (docstrings or comments), the line length should be limited to 72 characters.

The reason for this dates back to an original convention from Fortran and punch cards. as mentioned in this interesting discussion. From this reference, the reasoning behind 80 characters is:

One code line is recorded on one punch card.
A punch card has 80 columns.
Columns from 73 to 80 are used to record a sequence number of each card.

Therefore, 72 characters for "content".

For some time I thought that this was just legacy thinking and figured that 99 characters was totally fine since it's all getting rendered via Sphinx to html and that's what people typically read. However, there's more at play here and 75 turn out to be a fairly good compromise.

Why 75?

First, here's some Python source:

tmp.py

"""This is a class docstring.

#### Same text wrapped to 79 characters  #######################################

Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor
incididunt ut labore et dolore magna aliqua. Non consectetur a erat nam at
lectus. Arcu felis bibendum ut tristique et egestas. Id velit ut tortor pretium
viverra suspendisse.

"""


def docstr():
    """This function contains a docstring.

    This line is exactly 72 characters #################################
    Here is some lipsum text wrapped to 72 characters ##################

    Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
    eiusmod tempor incididunt ut labore et dolore magna aliqua. Non
    consectetur a erat nam at lectus. Arcu felis bibendum ut tristique
    et egestas. Id velit ut tortor pretium viverra suspendisse.

    This line is exactly 79 characters ########################################
    Same text wrapped to 79 characters  #######################################

    Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod
    tempor incididunt ut labore et dolore magna aliqua. Non consectetur a erat
    nam at lectus. Arcu felis bibendum ut tristique et egestas. Id velit ut
    tortor pretium viverra suspendisse.

    Same text wrapped to 99 characters  ###########################################################

    Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut
    labore et dolore magna aliqua. Non consectetur a erat nam at lectus. Arcu felis bibendum ut
    tristique et egestas. Id velit ut tortor pretium viverra suspendisse.

    """
    pass

class DocStr():

    def method_docstr():
        """This method contains a docstring.

        This line is exactly 72 characters #############################
        Here is some lipsum text wrapped to 72 characters ##############

        Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
        eiusmod tempor incididunt ut labore et dolore magna aliqua. Non
        consectetur a erat nam at lectus. Arcu felis bibendum ut
        tristique et egestas. Id velit ut tortor pretium viverra
        suspendisse.

        This line is exactly 79 characters ####################################
        Same text wrapped to 79 characters  ###################################

        Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod
        tempor incididunt ut labore et dolore magna aliqua. Non consectetur a
        erat nam at lectus. Arcu felis bibendum ut tristique et egestas. Id
        velit ut tortor pretium viverra suspendisse.

        """
        pass

    class NestedClass():
        """Nested class docstring.

        This line is exactly 72 characters #############################
        Here is some lipsum text wrapped to 72 characters ##############

        Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
        eiusmod tempor incididunt ut labore et dolore magna aliqua. Non
        consectetur a erat nam at lectus. Arcu felis bibendum ut
        tristique et egestas. Id velit ut tortor pretium viverra
        suspendisse.

        """
        pass

Let's see how this gets rendered right in the python REPL. For fun, let's use the cool-retro-term sized to the IBM 3270 (80x24):

>>> import tmp
>>> help(tmp.docstr)

Looks pretty good, right? This seems to make the argument that 79 characters (including the indentation) is just fine.

However, let's try this again when getting the help of the module:

>>> import tmp
>>> help(tmp)

Ok. So it seems that since everything is indented by four characters, why not just use 75 characters? This is actually what numpydoc recommends in https://numpydoc.readthedocs.io/en/latest/format.html:

The length of docstring lines should be kept to 75 characters to facilitate reading the docstrings in text terminals.

This even works for method docstrings. Behold:

Why then does PEP8 recommend 72? Part of it is a nod for the Fortran convention, and part of it is to allow a margin of four characters to the right of the terminal (for symmetry it seems). As you can see from above, 75 only has one character to the right of the text. However I'd argue that one character is good enough.

Should we just go with 72?

The question is: should reduce readability by limiting docstrings to 72 characters due to excessive wrapping using blackdoc? Is three characters really worth it?

I would argue that we should make the text as wide as possible before it is wrapped on a 80 character terminal. As shown above, this works for 75 characters and has been adopted by the numpydoc standard. This stays as close as possible to PEP8 while allowing for an extra three characters of "breathing room" to permit better formatted example sections.

Also, since we're adopting the numpydoc standard, let's align on that length.

Finally, I'd like to close with some remarks from PEP 257:

“A universal convention supplies all of maintainability, clarity, consistency, and a foundation for good programming habits too. What it doesn’t do is insist that you follow it against your will. That’s Python!”
—Tim Peters on comp.lang.python, 2001-06-16

This PR

This is a refactoring PR and the content will be ignored, we will document our standard in CONTRIBUTING.md.

[MatthewFlamm]

My personal thoughts are that usage of examples is in a contradictory state with this phrase:

For flowing long blocks of text with fewer structural restrictions (docstrings or comments), the line length should be limited to 72 characters.

I would argue that we might expect to be able to expand our line length when incorporating code into the docstring. But, given the usefulness of IDE hover hints that also suffer from line-length issues, I think limiting docstring length to some reasonable degree is desirable.

The below statement is the one that resonates most to me from your explanation for choosing a specific line length. I am less swayed by the terminal restrictions since I think it is very unlikely someone is using pyvista on such a terminal with specific restrictions in an interactive mode. Can those systems even install vtk version 9+? I think modern restrictions tend to be more amorphous (75 better for one user, and 79 better for another user).

Also, since we're adopting the numpydoc standard, let's align on that length.

[MatthewFlamm]

I am disappointed that docstrings are limited to doctest directives with comments inline. This causes a majority of the poor formatting in my reading, but I'm not sure how to fix those. I personally don't like when the lines have to be broken only be an inline comments, so I fixed those when I saw them.

[adeak]

I don't want to cause additional work to anyone, but we should discuss one thing: manual edits made here vs ignoring this PR in the git blame.

Option 1: we fix up all the uglies here. Pro: no additional work. Con: if we inadvertently break something it will be hidden from the git blame. Large issue in a very unlikely scenario.

Option 2: we only keep the autoformat in this PR (assuming we're OK with the diff here, pending manual surgical improvements). Pro: all human intervention will show up in the blame, complete with potential mistakes. Con: considerable amount of additional work of having to apply comments from @MatthewFlamm (thanks!) and potential future comments from others as a separate PR.

Option 3: anything I didn't think of.

What do you all think?

[akaszynski]

Voting option 1 with lots of testing.

[MatthewFlamm]

There is no perfect plan. If the changes are docstring format related, I think the impact is low. I vote for # 1 too. Although we have doctesting, they aren't designed primarily to be tests, so it is a soft backstop.

IMO users of git blame are more likely to want to search some functionality change backwards. So, being on the side of more hidden formatting changes might be beneficial.

It is always good to consider it to make an informed merge rather than regret it later.

[akaszynski]

Aside from the correction of the color channel wording (blue to green) I think that all these changes are merely style changes and I'm not really concerned about the blame recording these.

[codecov]

Codecov Report

Merging #4129 (062fe1a) into main (451b61c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #4129   +/-   ##
=======================================
  Coverage   95.56%   95.56%           
=======================================
  Files          95       95           
  Lines       20247    20247           
=======================================
  Hits        19350    19350           
  Misses        897      897           

[MatthewFlamm]

Thanks for addressing my comments. I like the np.hstack usage for forming the cells for UnstructuredGrid that you implemented. This makes the formatting better, and the concept easier to understand, win-win.

[adeak]

I'd have gone with np.concatenate instead of np.hstack, but I agree with all the changes. Thanks for the discussion, all!

[akaszynski]

I'd have gone with np.concatenate instead of np.hstack, but I agree with all the changes. Thanks for the discussion, all!

Since we convert to an array regardless, I actually could have gone with just adding the two lists together as well, as that would have been the most concise.