Wrong admonition reformat on v1.6.0.rc1
#157
Comments
|
OK, looking at the recent changes, it seems returning exit code 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 ? |
|
@kdeldycke I agree the admonition is what docformatter is tripping over. I removed the first regex in the Yes, the non-zero return code was requested to make pre-commit fail if docformatter id'd any changes in 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. |
weibullguy
added
P: bug
kdeldycke
changed the title
v1.6.0.rc1 always exits with code 3v1.6.0.rc1
|
Thanks @weibullguy for the fast reply! I'm currently test-driving In the mean time, I rename this issue to refer to admonition formatting as the |
|
Testing 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.
"""--- 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.
""" |
|
Another case of strange indention with 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>`.
"""--- 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>`.
"""
|
|
And for more details, see the result of |
|
@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 Also, are you OK with me using some of your docstrings as test inputs? |
|
Yes feel free to reuse my docstrings in your unittests. That's the spirit of open-source right? 🤗 I'll try |
|
OK, I stumbled upon a couple of edge-cases with Given the 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 --- 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:
|
|
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 . |
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. 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.
Agreed, it's ugly. I'll look into this.
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. |
|
Thanks @weibullguy for the feedback!
|
|
Just uncovered 2 more issues. Given the 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>`_.
"""
--- 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 The second is a new edge-case, in which a extra spaces are introduced as an side-effect of the nested functions. |
To keep the summary on the line after the
Agreed, shouldn't do this. I'm on it. |
|
Oh I didn't noticed the extra dot at the end of the summary. Good catch! And thanks for your work on |
All schedule pressure is self imposed ;). The latest tag |
|
Thanks @weibullguy for the new release candidate! Trying 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! :) |
|
Last round of issues I uncovered with 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.
"""--- 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 |
|
@kdeldycke Tag |
|
Thanks for Spotted what looks like the latests series of edge-cases I could identify: 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>`_.
"""--- 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:
|
Thanks for all your help with the testing!
I'm guessing that addressing your third point should address this as well.
Correct, per PEP-257, "There’s no blank line either before or after the docstring." so docformatter sees
It's already part way there by happenstance. For example, this docstring you provided earlier is no longer reformatted: but this one still is, whether the function is nested or not: 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. |
|
@kdeldycke v1.6.0-rc6 is available. |
Let's try that! |
|
Here is the new set of edge-cases: """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.
"""--- 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.
"""
|
|
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 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 v1.6.0-rc7 is ready if you're interested in testing it. |
|
Thanks for all the feedback! I'm good with all the decisions regarding the behaviour of Just tried Here is the test case: 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.
"""--- 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. |
I am using docformatter 1.5.0 for a couple of month now by invoking the following CLI, which always worked fine:
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 code3. See the log from my GitHub action:How to reproduce
First, install
v1.6.0.rc1:BTW, notice how the package is reported as
1.5.1bypipxand1.5.0bydocformatteritself. This is a confusing issue due to the1.6.0.rc1version string not being set in the development branch. Inspecting the code revealpipxis perfectly doing its job and that's really is thev1.6.0.rc1branch code we are running. For proof, the presence of theURL_PATTERNSconstant that was added in commit 917fed92248d1fb91a1f4da61b5aaaec7367fe23:Now that we run
1.6.0.rc1, let's download a minimal file from my project:And run
docformatteron it:Hypothesis
If we run the same command as above, on the same file, with the extra
--diffparameter, we get:Is the strange reformatting, which doesn't seems to recognize the
.. warning::admonition, can explain the constant exit with code3?The text was updated successfully, but these errors were encountered: