PyCQA / docformatter

Formats docstrings to follow PEP 257
https://pypi.python.org/pypi/docformatter
MIT License
526 stars 61 forks source link

Wrong admonition reformat on `v1.6.0.rc1` #157

Closed kdeldycke closed 1 year ago

kdeldycke commented 1 year ago

I am using docformatter 1.5.0 for a couple of month now by invoking the following CLI, which always worked fine:

$ docformatter --recursive --in-place --wrap-summaries 88 --wrap-descriptions 88 .

In order to test for the fix for #140, I updated to v1.6.0.rc1. Now the same call with the same parameters always exits with error code 3. See the log from my GitHub action:

$ docformatter --recursive --in-place --wrap-summaries 88 --wrap-descriptions 88 .
(...)
Error: Process completed with exit code 3.

How to reproduce

First, install v1.6.0.rc1:

$ pipx install git+https://github.com/PyCQA/docformatter.git@v1.6.0.rc1
  installed package docformatter 1.5.1, installed using Python 3.11.1
  These apps are now globally available
    - docformatter
done! ✨ 🌟 ✨
$ docformatter --version
docformatter 1.5.0

BTW, notice how the package is reported as 1.5.1 by pipx and 1.5.0 by docformatter itself. This is a confusing issue due to the 1.6.0.rc1 version string not being set in the development branch. Inspecting the code reveal pipx is perfectly doing its job and that's really is the v1.6.0.rc1 branch code we are running. For proof, the presence of the URL_PATTERNS constant that was added in commit 917fed92248d1fb91a1f4da61b5aaaec7367fe23:

$ grep -A 4 'URL_PATTERNS =' ~/.local/pipx/venvs/docformatter/lib/python3.11/site-packages/docformatter/syntax.py
URL_PATTERNS = (
    "afp|"
    "apt|"
    "bitcoin|"
    "chrome|"

Now that we run 1.6.0.rc1, let's download a minimal file from my project:

$ wget "https://raw.githubusercontent.com/kdeldycke/click-extra/86991e4023a962ff14e0ec774e534e118ad3b56e/click_extra/platform.py"
(...)
2023-01-24 (11.3 MB/s) - ‘platform.py’ saved

And run docformatter on it:

$ docformatter --recursive --in-place --wrap-summaries 88 --wrap-descriptions 88 ./platform.py
$ echo $?
3

Hypothesis

If we run the same command as above, on the same file, with the extra --diff parameter, we get:

$ docformatter --recursive --in-place --wrap-summaries 88 --wrap-descriptions 88 --diff ./platform.py
--- before/./platform.py
+++ after/./platform.py
@@ -21,9 +21,9 @@

 .. warning::

-    ``click_extra.platform`` is deprecated since version 3.8.0.
+``click_extra.platform`` is deprecated since version 3.8.0.

-    Use ``click_extra.platforms`` (with a trailing ``s``) instead.
+Use ``click_extra.platforms`` (with a trailing ``s``) instead.
 """

 import warnings
$ echo $?
3

Is the strange reformatting, which doesn't seems to recognize the .. warning:: admonition, can explain the constant exit with code 3?

kdeldycke commented 1 year ago

OK, looking at the recent changes, it seems returning exit code 3 is expected.

If that's the case, maybe I should requalify this issue, as the admonition formatting I pointed to as my last hypothesis is still valid right? What your opinion @weibullguy ?

weibullguy commented 1 year ago

@kdeldycke I agree the admonition is what docformatter is tripping over. I removed the first regex in the is_some_kind_of_list() function to fix the problem reported in #127. I just added a regex to that function to look specifically for admonitions and not process them. After v1.6.0 is released, I'm going to start tackling all the issues having to do with reST directives for v1.7.0.

Yes, the non-zero return code was requested to make pre-commit fail if docformatter id'd any changes in --check mode.

I just pushed the fix for the admonition and updated the version info. Tag v1.6.0-rc2 contains both fixes. Thanks for taking the time to test the RCs and provide feedback.

kdeldycke commented 1 year ago

Thanks @weibullguy for the fast reply! I'm currently test-driving v1.6.0-rc2 and will report issues.

In the mean time, I rename this issue to refer to admonition formatting as the 3 exit code is not a bug but a feature.

kdeldycke commented 1 year ago

Testing v1.6.0-rc2, I just stumbled upon a new bad case of admonition reformatting:

$ docformatter --version
docformatter 1.6.0-rc2
$ cat ./test.py
def test():
    """Introspects current CLI and list its parameters and metadata.

    .. important::
        Click doesn't keep a list of all parsed arguments and their origin.
        So we need to emulate here what's happening during CLI invokation.
        But can't even to that because the raw, pre-parsed arguments are
        not available anywhere.
    """
$ docformatter --wrap-summaries 88 --wrap-descriptions 88 --diff ./test.py
--- before/./test.py
+++ after/./test.py
@@ -1,9 +1,8 @@
 def test():
     """Introspects current CLI and list its parameters and metadata.

-    .. important::
-        Click doesn't keep a list of all parsed arguments and their origin.
-        So we need to emulate here what's happening during CLI invokation.
-        But can't even to that because the raw, pre-parsed arguments are
-        not available anywhere.
+    .. important::     Click doesn't keep a list of all parsed arguments and their
+    origin.     So we need to emulate here what's happening during CLI invokation.
+    But can't even to that because the raw, pre-parsed arguments are     not available
+    anywhere.
     """
kdeldycke commented 1 year ago

Another case of strange indention with v1.6.0-rc2:

$ cat ./test2.py
def test2():
    """Search on local file system or remote URL files matching the provided pattern.

    ``pattern`` is considered as an URL only if it is parseable as such
    and starts with ``http://`` or ``https://``.

    .. important::

        This is a straight `copy of the functools.cache implementation
        <https://github.com/python/cpython/blob/55a26de6ba938962dc23f2495723cf0f4f3ab7c6/Lib/functools.py#L647-L653>`_,
        which is only `available in the standard library starting with Python v3.9
        <https://docs.python.org/3/library/functools.html?highlight=caching#functools.cache>`.
    """
$ docformatter --wrap-summaries 88 --wrap-descriptions 88 --diff ./test2.py
--- before/./test.py
+++ after/./test.py
@@ -1,13 +1,17 @@
 def test2():
     """Search on local file system or remote URL files matching the provided pattern.

-    ``pattern`` is considered as an URL only if it is parseable as such
-    and starts with ``http://`` or ``https://``.
+    ``pattern`` is considered as an URL only if it is parseable as such and starts with
+    ``
+    http://``
+    or ``https://``.

     .. important::

-        This is a straight `copy of the functools.cache implementation
-        <https://github.com/python/cpython/blob/55a26de6ba938962dc23f2495723cf0f4f3ab7c6/Lib/functools.py#L647-L653>`_,
-        which is only `available in the standard library starting with Python v3.9
-        <https://docs.python.org/3/library/functools.html?highlight=caching#functools.cache>`.
+    This is a straight `copy of the functools.cache implementation
+    <https://github.com/python/cpython/blob/55a26de6ba938962dc23f2495723cf0f4f3ab7c6/Lib
+    /functools.py#L647-L653>`_,
+    which is only `available in the standard library starting with Python v3.9
+
+    <https://docs.python.org/3/library/functools.html?highlight=caching#functools.cache>`.
     """
kdeldycke commented 1 year ago

And for more details, see the result of v1.6.0-rc2 on my code in this PR: https://github.com/kdeldycke/click-extra/pull/416/files#diff-673e6a13811183399e9e9ad5e497bbd63e560f594b395ad8086e5d39df3ba728

weibullguy commented 1 year ago

@kdeldycke OK. I added a function to find and ignore reST directives (for now). Once I get all this URL business straightened out, I'll start working on docformatter actually handling reST directives properly. In the meantime, you can try v1.6.0-rc3 and see if there's any improvement.

Also, are you OK with me using some of your docstrings as test inputs?

kdeldycke commented 1 year ago

Yes feel free to reuse my docstrings in your unittests. That's the spirit of open-source right? 🤗

I'll try v1.6.0-rc3 and check its effects.

kdeldycke commented 1 year ago

OK, I stumbled upon a couple of edge-cases with 1.6.0-rc3:

$ docformatter --version
docformatter 1.6.0-rc3

Given the docformatter-1.6.0-rc3-tests.py file, containing:

def valid_rest_link_being_realigned():
    """Locate and call the ``mpm`` CLI.

    The output must supports both `Xbar dialect
    <https://github.com/matryer/xbar-plugins/blob/main/CONTRIBUTING.md#plugin-api>`_
    and `SwiftBar dialect <https://github.com/swiftbar/SwiftBar#plugin-api>`_.
    """

def multiline_paragraphs_with_long_lines():
    """Install one or more packages.

    Installation will proceed first with packages unambiguously tied to a manager. You can have an
    influence on that with more precise package specifiers (like purl) and/or tighter selection of managers.

    For other untied packages, mpm will try to find the best manager to install it with. Their installation
    will be attempted with each manager, in the order they were selected. If we have the certainty, by the way
    of a search operation, that this package is not available from this manager, we'll skip the installation
    and try the next available manager.
    """

docstring_starting_with_an_admonition = True
"""
.. code-block:: shell-session

    ► apm --version
    apm  2.6.2
    npm  6.14.13
    node 12.14.1 x64
    atom 1.58.0
    python 2.7.16
    git 2.33.0
"""

Here is how docformatter 1.6.0-rc3 handle it:

$ docformatter --wrap-summaries 88 --wrap-descriptions 88 --diff ./docformatter-1.6.0-rc3-tests.py
--- before/./docformatter-1.6.0-rc3-tests.py
+++ after/./docformatter-1.6.0-rc3-tests.py
@@ -1,34 +1,34 @@
 def valid_rest_link_being_realigned():
     """Locate and call the ``mpm`` CLI.

-    The output must supports both `Xbar dialect
-    <https://github.com/matryer/xbar-plugins/blob/main/CONTRIBUTING.md#plugin-api>`_
+    The output must supports both
+    `Xbar dialect <https://github.com/matryer/xbar-plugins/blob/main/CONTRIBUTING.md#plugin-api>`_
     and `SwiftBar dialect <https://github.com/swiftbar/SwiftBar#plugin-api>`_.
     """
-
-
 def multiline_paragraphs_with_long_lines():
     """Install one or more packages.

-    Installation will proceed first with packages unambiguously tied to a manager. You can have an
-    influence on that with more precise package specifiers (like purl) and/or tighter selection of managers.
+    Installation will proceed first with packages unambiguously tied to a manager. You
+    can have an
+    influence on that with more precise package specifiers (like purl) and/or tighter
+    selection of managers.

-    For other untied packages, mpm will try to find the best manager to install it with. Their installation
-    will be attempted with each manager, in the order they were selected. If we have the certainty, by the way
-    of a search operation, that this package is not available from this manager, we'll skip the installation
+    For other untied packages, mpm will try to find the best manager to install it with.
+    Their installation
+    will be attempted with each manager, in the order they were selected. If we have the
+    certainty, by the way
+    of a search operation, that this package is not available from this manager, we'll
+    skip the installation
     and try the next available manager.
     """
+docstring_starting_with_an_admonition = True
+""".. code-block:: shell-session.

-
-docstring_starting_with_an_admonition = True
+► apm --version
+apm  2.6.2
+npm  6.14.13
+node 12.14.1 x64
+atom 1.58.0
+python 2.7.16
+git 2.33.0
 """
-.. code-block:: shell-session
-
-    ► apm --version
-    apm  2.6.2
-    npm  6.14.13
-    node 12.14.1 x64
-    atom 1.58.0
-    python 2.7.16
-    git 2.33.0
-"""

Instead, here is what I expect:

  1. The first def valid_rest_link_being_realigned(): function should be left as-is. Because it respects the 88-long line constraint. See how I split the URL link at the last space, before the < character. This layout is valid in reST and properly renders in Sphinx. It allows for longer URLs.
  2. The second def multiline_paragraphs_with_long_lines(): function reflows all paragraphs because they're too long. That's good. But there's an opportunity missed here to re-compose them entirely and get rid of the early mid-sentence splits (see the Their installation line).
  3. The last docstring_starting_with_an_admonition = True variable possess, I admit, an ugly docstring composed of a single reST code block. Still, this is valid reST and renders properly in Sphinx. Unlike docformatter pass, which breaks the structure and indention of the code-block.
kdeldycke commented 1 year ago

The test cases above where taken from https://github.com/kdeldycke/meta-package-manager/pull/917/files and https://github.com/kdeldycke/click-extra/pull/416/files .

weibullguy commented 1 year ago

@kdeldycke

  1. The first def valid_rest_link_being_realigned(): function should be left as-is. Because it respects the 88-long line constraint. See how I split the URL link at the last space, before the < character. This layout is valid in reST and properly renders in Sphinx. It allows for longer URLs.

I made a design decision to keep reST in-line links all together. So the "URL" in the following begins with the first back tick which is why it produces the result in does.

`Xbar dialect
<https://github.com/matryer/xbar-plugins/blob/main/CONTRIBUTING.md#plugin-api>`_

The problem was, I was getting the following and couldn't find a good solution that would keep the `_ with the actual URL and I think this looks even worse.

`Xbar dialect
<https://github.com/matryer/xbar-plugins/blob/main/CONTRIBUTING.md#plugin-api>
`_
  1. The second def multiline_paragraphs_with_long_lines(): function reflows all paragraphs because they're too long. That's good. But there's an opportunity missed here to re-compose them entirely and get rid of the early mid-sentence splits (see the Their installation line).

Agreed, it's ugly. I'll look into this.

  1. The last docstring_starting_with_an_admonition = True variable possess, I admit, an ugly docstring composed of a single reST code block. Still, this is valid reST and renders properly in Sphinx. Unlike docformatter pass, which breaks the structure and indention of the code-block.

This is because the reST directive is in the summary line of the multi-line docstring, not the description. The function I added that is ignoring reST directives is only called when processing the description. I'll have to call it when processing the summary as well.

kdeldycke commented 1 year ago

Thanks @weibullguy for the feedback!

  1. For this case I guess there is no quick fix, hence your reasonable compromise. In the mean-time I'm annoyed a bit as ruff keeps reporting an E501 Line too long (113 > 88 characters) error for that case. I'll try to look at the code to see if there's a non-invasive workaround we can come up with.
  2. Thanks! :)
  3. OK I see! :)
kdeldycke commented 1 year ago

Just uncovered 2 more issues.

Given the docformatter-1.6.0-rc3-tests-2.py file, containing:

def hanging_rest_link():
    """
    `Source of this snippet
    <https://www.freecodecamp.org/news/how-to-flatten-a-dictionary-in-python-in-4-different-ways/>`_.
    """

def sub_func_test():

    def long_line_link():
        """Get the Python type of a Click parameter.

        See the list of `custom types provided by Click
        <https://click.palletsprojects.com/en/8.1.x/api/?highlight=intrange#types>`_.
        """

docformatter 1.6.0-rc3 produces:

$ docformatter --wrap-summaries 88 --wrap-descriptions 88 --diff ./docformatter-1.6.0-rc3-tests-2.py
--- before/./docformatter-1.6.0-rc3-tests-2.py
+++ after/./docformatter-1.6.0-rc3-tests-2.py
@@ -1,15 +1,12 @@
 def hanging_rest_link():
-    """
-    `Source of this snippet
+    """`Source of this snippet.
+
     <https://www.freecodecamp.org/news/how-to-flatten-a-dictionary-in-python-in-4-different-ways/>`_.
     """
-
 def sub_func_test():
-
     def long_line_link():
         """Get the Python type of a Click parameter.

-        See the list of `custom types provided by Click
-        <https://click.palletsprojects.com/en/8.1.x/api/?highlight=intrange#types>`_.
+        See the list of
+        `custom types provided by Click        <https://click.palletsprojects.com/en/8.1.x/api/?highlight=intrange#types>`_.
         """
-

The first case looks like third case of my previous tests. I.e., to quote you, because the reST directive is in the summary line of the multi-line docstring, not the description. I guess you'll have this sorted out once you plug the reST-aware ignoring code to the summary code path.

The second is a new edge-case, in which a extra spaces are introduced as an side-effect of the nested functions.

weibullguy commented 1 year ago

def hanging_restlink(): """ Source of this snippet <https://www.freecodecamp.org/news/how-to-flatten-a-dictionary-in-python-in-4-different-ways/>. """

To keep the summary on the line after the """, you'd need to pass the --pre-summary-newline argument. However, that's global. But, the . being added after snippet is the result of it being recognized as the summary line. I'll look into that.

The second is a new edge-case, in which a extra spaces are introduced as an side-effect of the nested functions.

Agreed, shouldn't do this. I'm on it.

kdeldycke commented 1 year ago

Oh I didn't noticed the extra dot at the end of the summary. Good catch!

And thanks for your work on docformatter. Take your time, there's no rush! :)

weibullguy commented 1 year ago

Take your time, there's no rush! :)

All schedule pressure is self imposed ;).

The latest tag v1.6.0-rc4 should address the few things you've reported in the past couple of comments. I await the next batch of problems you find :)

kdeldycke commented 1 year ago

Thanks @weibullguy for the new release candidate! Trying v1.6.0-rc4 right now on all my codebase.

Just a nitpick, you forgot to bump the version in: https://github.com/PyCQA/docformatter/blob/v1.6.0-rc4/pyproject.toml#L3

Will report on new issues! :)

kdeldycke commented 1 year ago

Last round of issues I uncovered with v1.6.0-rc4.

Here are some additional cases, mainly around wrapping of links. Also added some definition list test case:

def mixed_links():
    """Implements the minimal code necessary to locate and call the ``mpm`` CLI on the
    system.

    Once ``mpm`` is located, we can rely on it to produce the main output of the plugin.

    The output must supports both `Xbar dialect
    <https://github.com/matryer/xbar-plugins/blob/main/CONTRIBUTING.md#plugin-api>`_
    and `SwiftBar dialect <https://github.com/swiftbar/SwiftBar#plugin-api>`_.
    """

XKCD_MANAGER_ORDER = ("pip", "brew", "npm", "dnf", "apt", "steamcmd")
"""Sequence of package managers as defined by `XKCD #1654: Universal Install Script
<https://xkcd.com/1654/>`_.

See the corresponding :issue:`implementation rationale in issue #10 <10>`.
"""

HASH_HEADERS = (
    "Date",
    "From",
    "To",
)
"""
Default ordered list of headers to use to compute the unique hash of a mail.

By default we choose to exclude:

``Cc``
  Since ``mailman`` apparently `sometimes trims list members
  <https://mail.python.org/pipermail/mailman-developers/2002-September/013233.html>`_
  from the ``Cc`` header to avoid sending duplicates. Which means that copies of mail
  reflected back from the list server will have a different ``Cc`` to the copy saved by
  the MUA at send-time.

``Bcc``
  Because copies of the mail saved by the MUA at send-time will have ``Bcc``, but copies
  reflected back from the list server won't.

``Reply-To``
  Since a mail could be ``Cc``'d to two lists with different ``Reply-To`` munging
  options set.
"""
$ docformatter --wrap-summaries 88 --wrap-descriptions 88 --diff ./docformatter-1.6.0-rc4-tests.py
--- before/./docformatter-1.6.0-rc4-tests.py
+++ after/./docformatter-1.6.0-rc4-tests.py
@@ -4,13 +4,14 @@

     Once ``mpm`` is located, we can rely on it to produce the main output of the plugin.

-    The output must supports both `Xbar dialect
-    <https://github.com/matryer/xbar-plugins/blob/main/CONTRIBUTING.md#plugin-api>`_
+    The output must supports both
+
+    `Xbar dialect <https://github.com/matryer/xbar-plugins/blob/main/CONTRIBUTING.md#plugin-api>`_
     and `SwiftBar dialect <https://github.com/swiftbar/SwiftBar#plugin-api>`_.
     """
+XKCD_MANAGER_ORDER = ("pip", "brew", "npm", "dnf", "apt", "steamcmd")
+"""Sequence of package managers as defined by `XKCD #1654: Universal Install Script.

-XKCD_MANAGER_ORDER = ("pip", "brew", "npm", "dnf", "apt", "steamcmd")
-"""Sequence of package managers as defined by `XKCD #1654: Universal Install Script
 <https://xkcd.com/1654/>`_.

 See the corresponding :issue:`implementation rationale in issue #10 <10>`.
@@ -21,15 +22,14 @@
     "From",
     "To",
 )
-"""
-Default ordered list of headers to use to compute the unique hash of a mail.
+"""Default ordered list of headers to use to compute the unique hash of a mail.

 By default we choose to exclude:

-``Cc``
-  Since ``mailman`` apparently `sometimes trims list members
-  <https://mail.python.org/pipermail/mailman-developers/2002-September/013233.html>`_
-  from the ``Cc`` header to avoid sending duplicates. Which means that copies of mail
+``Cc``   Since ``mailman`` apparently
+
+`sometimes trims list members  <https://mail.python.org/pipermail/mailman-developers/2002-September/013233.html>`_
+from the ``Cc`` header to avoid sending duplicates. Which means that copies of mail
   reflected back from the list server will have a different ``Cc`` to the copy saved by
   the MUA at send-time.

I guess the next release candidate rc5 will be the last. docformatter is getting pretty solid with each iteration. And with all the test cases I provided you should have a complete corpus that will make docformatter stable. Keep up the good work @weibullguy and thanks a lot for your patience!

weibullguy commented 1 year ago

@kdeldycke Tag v1.6.0-rc5 is available.

kdeldycke commented 1 year ago

Thanks for v1.6.0-rc5!

Spotted what looks like the latests series of edge-cases I could identify:

$ docformatter --version
docformatter 1.6.0-rc5
def load_conf():
    """Fetch parameters values from configuration file and merge them with the
    defaults.

    User configuration is `merged to the context default_map as Click does
    <https://click.palletsprojects.com/en/8.1.x/commands/#context-defaults>`_.

    This allow user's config to only overrides defaults. Values sets from direct
    command line parameters, environment variables or interactive prompts, takes
    precedence over any values from the config file.
    """

strict_selection_match = False
"""
Install sub-command try each user-selected manager until it find one providing
the package we seek to install, after which the process stop. This mean not all
managers will be called, so we allow the CLI output checks to partially match.
"""

platforms = {"LINUX", "MACOS", "WSL2"}
"""Homebrew core is now compatible with `Linux and Windows Subsystem for Linux
(WSL) 2 <https://docs.brew.sh/Homebrew-on-Linux>`_.
"""
$ docformatter --wrap-summaries 88 --wrap-descriptions 88 --diff ./docformatter-1.6.0-rc5-tests.py
--- before/./docformatter-1.6.0-rc5-tests.py
+++ after/./docformatter-1.6.0-rc5-tests.py
@@ -1,25 +1,21 @@
 def load_conf():
-    """Fetch parameters values from configuration file and merge them with the
-    defaults.
+    """Fetch parameters values from configuration file and merge them with the defaults.

-    User configuration is `merged to the context default_map as Click does
-    <https://click.palletsprojects.com/en/8.1.x/commands/#context-defaults>`_.
-
+    User configuration is
+    `merged to the context default_map as Click does <https://click.palletsprojects.com/en/8.1.x/commands/#context-defaults>`_.
     This allow user's config to only overrides defaults. Values sets from direct
     command line parameters, environment variables or interactive prompts, takes
     precedence over any values from the config file.
     """
+strict_selection_match = False
+"""Install sub-command try each user-selected manager until it find one providing the
+package we seek to install, after which the process stop.

-
-strict_selection_match = False
-"""
-Install sub-command try each user-selected manager until it find one providing
-the package we seek to install, after which the process stop. This mean not all
-managers will be called, so we allow the CLI output checks to partially match.
+This mean not all managers will be called, so we allow the CLI output checks to
+partially match.
 """

 platforms = {"LINUX", "MACOS", "WSL2"}
-"""Homebrew core is now compatible with `Linux and Windows Subsystem for Linux
-(WSL) 2 <https://docs.brew.sh/Homebrew-on-Linux>`_.
-"""
+"""Homebrew core is now compatible with `Linux and Windows Subsystem for Linux (WSL) 2
+<https://docs.brew.sh/Homebrew-on-Linux>`_."""

As you can see in the test-cases above:

  1. The re-alignment of the reST link is removing the blank line between paragraphs.
  2. The strict_selection_match statement gets collated to the empty function above. But that might be a fault of my own, as the load_conf() function is empty and should have contained at least one Python statement (like pass or ...). Also not how the first line is used as a summary and the rest as the body of the docstring. But again that is indeed a smart feature of docformatter.
  3. Link in summary is allowed to be split in the middle of its statement, unlike in the body of the docstring. It seems you already have the code somewhere to do that in the summary, so having this supported in the body might be possible with some generalization of that code.
weibullguy commented 1 year ago

Thanks for v1.6.0-rc5!

Thanks for all your help with the testing!

  1. The re-alignment of the reST link is removing the blank line between paragraphs.

I'm guessing that addressing your third point should address this as well.

  1. The strict_selection_match statement gets collated to the empty function above. But that might be a fault of my own, as the load_conf() function is empty and should have contained at least one Python statement (like pass or ...). Also not how the first line is used as a summary and the rest as the body of the docstring. But again that is indeed a smart feature of docformatter.

Correct, per PEP-257, "There’s no blank line either before or after the docstring." so docformatter sees strict_selection_match = False as the first line of code in the function. Add a pass or other statement and the this shouldn't happen

  1. Link in summary is allowed to be split in the middle of its statement, unlike in the body of the docstring. It seems you already have the code somewhere to do that in the summary, so having this supported in the body might be possible with some generalization of that code.

It's already part way there by happenstance. For example, this docstring you provided earlier is no longer reformatted:

def valid_rest_link_being_realigned():
    """Locate and call the ``mpm`` CLI.

    The output must supports both `Xbar dialect
    <https://github.com/matryer/xbar-plugins/blob/main/CONTRIBUTING.md#plugin-api>`_
    and `SwiftBar dialect <https://github.com/swiftbar/SwiftBar#plugin-api>`_.
    """

but this one still is, whether the function is nested or not:

def long_line_link():
    """Get the Python type of a Click parameter.

    See the list of `custom types provided by Click
    <https://click.palletsprojects.com/en/8.1.x/api/?highlight=intrange#types>`_.
    """

I don't see any obvious differences except the third line in the first and the period at the end of the second. But, if I removed the period and add a third line to the second, it still reformats. Weird.

weibullguy commented 1 year ago

@kdeldycke v1.6.0-rc6 is available.

kdeldycke commented 1 year ago

@kdeldycke v1.6.0-rc6 is available.

Let's try that!

kdeldycke commented 1 year ago

Here is the new set of edge-cases:

$ docformatter --version
docformatter 1.6.0-rc6

$ cat ./docformatter-1.6.0-rc6-tests.py
"""Patch and tweak `Python's standard library mail box constructors.

<https://docs.python.org/3.11/library/mailbox.html>`_ to set sane defaults.

Also forces out our own message factories to add deduplication tools and utilities.
"""

"""Patch and tweak `Python's standard library mail box constructors
<https://docs.python.org/3.11/library/mailbox.html>`_ to set sane defaults.

Also forces out our own message factories to add deduplication tools and utilities.
"""

def generate_platforms_graph(
    graph_id: str, description: str, groups: frozenset
) -> str:
    """Generates an `Euler diagram <https://xkcd.com/2721/>`_ of platform and their
    grouping.

    Euler diagrams are
    `not supported by mermaid yet <https://github.com/mermaid-js/mermaid/issues/2583>`_
    so we fallback on a flowchart
    without arrows.

    Returns a ready to use and properly indented MyST block.
    """

def load_conf(self, ctx, param, path_pattern):
    """Fetch parameters values from configuration file and merge them with the
    defaults.

    User configuration is `merged to the context default_map as Click does
    <https://click.palletsprojects.com/en/8.1.x/commands/#context-defaults>`_.

    This allow user's config to only overrides defaults. Values sets from direct
    command line parameters, environment variables or interactive prompts, takes
    precedence over any values from the config file.
    """

def pytest_addoption(parser):
    """Add custom command line options.

    Based on `Pytest's documentation examples
    <https://docs.pytest.org/en/latest/example/simple.html#control-skipping-of-tests-according-to-command-line-option>`_.

    By default, runs non-destructive tests and skips destructive ones.
    """
$ docformatter --wrap-summaries 88 --wrap-descriptions 88 --diff ./docformatter-1.6.0-rc6-tests.py > diff
--- before/./docformatter-1.6.0-rc6-tests.py
+++ after/./docformatter-1.6.0-rc6-tests.py
@@ -1,7 +1,6 @@
 """Patch and tweak `Python's standard library mail box constructors.

-<https://docs.python.org/3.11/library/mailbox.html>`_ to set sane defaults.
-
+https://docs.python.org/3.11/library/mailbox.html>`_ to set sane defaults.
 Also forces out our own message factories to add deduplication tools and utilities.
 """

@@ -23,30 +22,24 @@
     `not supported by mermaid yet <https://github.com/mermaid-js/mermaid/issues/2583>`_
     so we fallback on a flowchart
     without arrows.
-
     Returns a ready to use and properly indented MyST block.
     """

 def load_conf(self, ctx, param, path_pattern):
-    """Fetch parameters values from configuration file and merge them with the
-    defaults.
+    """Fetch parameters values from configuration file and merge them with the defaults.

-    User configuration is `merged to the context default_map as Click does
-    <https://click.palletsprojects.com/en/8.1.x/commands/#context-defaults>`_.
-
-
+    User configuration is
+    `merged to the context default_map as Click does <https://click.palletsprojects.com/en/8.1.x/commands/#context-defaults>`_.
+    
     This allow user's config to only overrides defaults. Values sets from direct
     command line parameters, environment variables or interactive prompts, takes
     precedence over any values from the config file.
     """
-
-
 def pytest_addoption(parser):
     """Add custom command line options.

     Based on `Pytest's documentation examples
     <https://docs.pytest.org/en/latest/example/simple.html#control-skipping-of-tests-according-to-command-line-option>`_.
-
     By default, runs non-destructive tests and skips destructive ones.
     """
weibullguy commented 1 year ago

@kdeldycke

The first docstring is a summary that has a line break in it. That would require potentially sizable changes and I think that needs to be addressed when I tackle reST directive handling. For now I fixed docformatter removing the < at the beginning of the URL and the blank line after.

Same with removing the blank line in the third and fifth docstrings.

In the fourth docstring I toyed with the idea of collapsing the two blank lines into one, but chose not too since some user may desire two blank lines. Maybe I'll rethink that in the future.

The blank lines after each docstring are removed because there's no Python statement. Add a pass and this won't happen.

v1.6.0-rc7 is ready if you're interested in testing it.

kdeldycke commented 1 year ago

Thanks for all the feedback! I'm good with all the decisions regarding the behaviour of v1.6.0-rc6.

Just tried v1.6.0-rc7, and everything's good to me. But for one thing: the inlining of URLs introduce superfluous indentation of the next blank line. This was already present in v1.6.0-rc6 as you can see in my previous example, but we both missed it.

Here is the test case:

$ docformatter --version
docformatter 1.6.0-rc7

$ cat ./docformatter-1.6.0-rc7-tests.py
def load_conf(self, ctx, param, path_pattern):
    """Fetch parameters values from configuration file and merge them with the defaults.

    User configuration is `merged to the context default_map as Click does
    <https://click.palletsprojects.com/en/8.1.x/commands/#context-defaults>`_.

    This allow user's config to only overrides defaults. Values sets from direct
    command line parameters, environment variables or interactive prompts, takes
    precedence over any values from the config file.
    """
$ docformatter --wrap-summaries 88 --wrap-descriptions 88 --diff ./docformatter-1.6.0-rc7-tests.py
--- before/./docformatter-1.6.0-rc7-tests.py
+++ after/./docformatter-1.6.0-rc7-tests.py
@@ -1,9 +1,9 @@
 def load_conf(self, ctx, param, path_pattern):
     """Fetch parameters values from configuration file and merge them with the defaults.

-    User configuration is `merged to the context default_map as Click does
-    <https://click.palletsprojects.com/en/8.1.x/commands/#context-defaults>`_.
-
+    User configuration is
+    `merged to the context default_map as Click does <https://click.palletsprojects.com/en/8.1.x/commands/#context-defaults>`_.
+    
     This allow user's config to only overrides defaults. Values sets from direct
     command line parameters, environment variables or interactive prompts, takes
     precedence over any values from the config file.