Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

""" getting placed at end of comment line instead of its own line #9

Closed
dwiel opened this issue Oct 13, 2015 · 11 comments · Fixed by #104
Closed

""" getting placed at end of comment line instead of its own line #9

dwiel opened this issue Oct 13, 2015 · 11 comments · Fixed by #104
Labels
C: convention Relates to docstring format convention P: enhancement Feature that is outside the scope of PEP 257

Comments

@dwiel
Copy link

dwiel commented Oct 13, 2015

docformatter is resulting in this diff:

-    """
-    return a map mapping appliance_ids for a house back to themselves
-    except for multiphase appliances
-    """
+    """return a map mapping appliance_ids for a house back to themselves except
+    for multiphase appliances."""

I'm ok with putting the word return immediately after the triple quotes, but shouldn't the final set of triple quotes be on their own line?

@dwiel
Copy link
Author

dwiel commented Oct 13, 2015

Quote from PEP 257: Unless the entire docstring fits on a line, place the closing quotes on a line by themselves.

@myint
Copy link
Member

myint commented Oct 13, 2015

It has been a few years since I initially interpreted PEP 257 so I looked through it again.

Isn't your quote from the multi-line docstring section? And it defines the multi-line case as "a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description". In this case, I see a summary, but no description.

I think in the past, I've considered this kind of a degenerate case. Typically, the summary should fit on one line. I'd be interested to see what the common usage in the modern parts of the standard library is since PEP 257 doesn't seem to show an example that clarifies this case. And if I recall, pep257, the tool, doesn't like either styles since the summary doesn't fit on one line.

@dwiel
Copy link
Author

dwiel commented Oct 13, 2015

hmm, yeah I see that now too. Definitely looks contradictory, but I didn't fully understand the distinction between the summary and the rest either.

I checked some random file from cpython and it is different in different places:

https://github.com/python/cpython/blob/master/Tools/importbench/importbench.py

def bench(name, cleanup=lambda: None, *, seconds=1, repeat=3):
    """Bench the given statement as many times as necessary until total
    executions take one second."""
    stmt = "__import__({!r})".format(name)

https://github.com/python/cpython/blob/master/Tools/pybench/CommandLine.py:

def _getopt_flags(options):

    """ Convert the option list to a getopt flag string and long opt
        list
    """

and from the same file:

def option_dict(options):

    """ Return a dictionary mapping option names to Option instances.
    """
    d = {}

@dwiel
Copy link
Author

dwiel commented Oct 13, 2015

doh, those examples were all from 2005-2006

@dwiel
Copy link
Author

dwiel commented Oct 13, 2015

Here is one from 2013:

    def _set_tstate_lock(self):
        """
        Set a lock object which will be released by the interpreter when
        the underlying thread state (see pystate.h) gets deleted.
        """
        self._tstate_lock = _set_sentinel()

oh well, sorry, its not as clear as I had thought.

@myint
Copy link
Member

myint commented Oct 13, 2015

I'm willing to accept a pull request to support this case. You can see the existing --pre-summary-newline for an example. I think previous (pull) requests were the reason that option and similar ones were added in the past.

@myint myint added the P: enhancement Feature that is outside the scope of PEP 257 label May 27, 2016
@Hnasar
Copy link

Hnasar commented Mar 30, 2018

I'd like to use this tool (since it seems really great!) but this issue is preventing me.

For example:

 def _iter_children(path):
     # type: (PathStr) -> Iterator[Tuple[PathStr, bool]]
-    """Visits file objects that are immediate children
-    Return a generator that iterates over (path, isdir)
-    """
+    """Visits file objects that are immediate children Return a generator that
+    iterates over (path, isdir)"""

The original docstring was missing a period and a newline. I had assumed that docformatter would instead produce:

def _iter_children(path):
    # type: (PathStr) -> Iterator[Tuple[PathStr, bool]]
    """Visits file objects that are immediate children.

    Return a generator that iterates over (path, isdir)
    """

@Nodd
Copy link

Nodd commented Mar 30, 2020

@Hnasar The program can't guess that there was a missing newline. However I think it should become :

 def _iter_children(path):
     # type: (PathStr) -> Iterator[Tuple[PathStr, bool]]
     """Visits file objects that are immediate children Return a generator that
     iterates over (path, isdir)
     """

@weibullguy
Copy link
Member

Per PEP-257, a one-line docstring "should really fit on one line" and "The closing quotes are on the same line as the opening quotes." Thus, the current docformatter behavior is in compliance with PEP-257.

This

"""
return a map mapping appliance_ids for a house back to themselves
except for multiphase appliances
"""

is being converted to a PEP-257 compliant one-line docstring by the normalize_summary function

"""return a map mapping appliance_ids for a house back to themselves except for multiphase appliances."""

which is then being broken after the word except based on the wrap-summaries argument by the wrap_summary function to achieve the final result

"""return a map mapping appliance_ids for a house back to themselves except
for multiphase appliances."""

Thus, there are two separate requirements interacting here.

Since the PEP-257 requirement "should really fit on one line" is a should, not a shall, requirement, I'd recommend the following:

  • Leave current docformatter behavior as-is. This is the strongest compliance with PEP-257.
  • Add an option (e.g., --close-quotes-on-newline) to place the closing and/or opening triple quotes on a new line whenever the summary line is broken. This seems a common enough preference (see also Bug: Multi-line summary has closing quotes on the same line #84) that it should be supported.

@Nodd
Copy link

Nodd commented Jul 24, 2022

My interpretation is that

"""return a map mapping appliance_ids for a house back to themselves except
for multiphase appliances."""

doesn't fit in one line, so it's not a one-line docstring but a 2-lines docstring. What applies is thus the multiline dosctring section, where he triple quotes should be on a separate line.

This is a one-line docstring, it fits on exactly one line:

"""return a map mapping appliance_ids."""

If the wrap_summary() function breaks at least one line, then it should ensure that the triple quotes are put on a separate line.

@weibullguy
Copy link
Member

My interpretation is that

I agree, PEP-257 is open to interpretation. That's why I'd argue where the closing quotes belong is ultimately a style choice. As a style choice it should be a user option, not something hard-coded.

PEP-257 is silent on the matter of line wrapping. So, line wrapping isn't being driven by PEP-257. The commit that added the summary wrapping supprt had the following message, "Add optional summary wrapping support" I can only speculate why it was added, but I'd guess it was to allow docformatter to wrap docstrings consistent with any code wrapping done by other tools.

That's why I'm saying there are competing requirements. Since docformatter is a tool to autoformat docstrings in accordance with PEP-257, the PEP-257 requirements should probably take precedence.

Per PEP-257, a one-line docstring "should fit on one line", "closing quotes are on the same line as the opening quotes", "no blank line either before or after the docstring", and "is a phrase ending in a period." Thus,

"""return a map mapping appliance_ids for a house back to themselves except
for multiphase appliances."""

meets the definition of a one-line docstring.

A multi-line docstring consists of "a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description." In a multi-line docstring, the summary line needs to meet the requirements of a one-line docstring. This doesn't imply that a one-line docstring is the same as a summary line; the summary line is a sub-class of the one-line docstring. One might argue the --wrap-summaries option shouldn't touch one-line docstrings at all.

Regardless, wrapping the line for visual consistency doesn't convert a one-line docstring to a multi-line docstring in my view. It's a one-line docstring that doesn't fit on one line even though it "should fit on one line". Because all the conditions of a one-line are met, the summary line condition of a multi-line is met, but none of the other conditions are. That's why I interpret it as a one-line.

Since PEP-257 doesn't require the docstring to be wrapped, I believe it's most consistent to leave the closing quotes "in line" with the words. Whether or not to do that or to place them on the next line by themselves is a style choice since neither is wrong per se. It depends on how you interpret the wrapped docstring, as a multi-line or a wrapped one-line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: convention Relates to docstring format convention P: enhancement Feature that is outside the scope of PEP 257
Projects
None yet
5 participants