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

duplicate content after running docformatter #263

Open
finswimmer opened this issue Aug 15, 2023 · 2 comments · May be fixed by #284
Open

duplicate content after running docformatter #263

finswimmer opened this issue Aug 15, 2023 · 2 comments · May be fixed by #284
Labels
C: style Relates to docstring format style (e.g., Google, NumPy, Sphinx) P: bug PEP 257 violation or existing functionality that doesn't work as documented U: medium A relatively medium urgency issue

Comments

@finswimmer
Copy link

Hey,

I've updated docformatter to 1.7.5 and discovered a strange behavior.

Given this minimal example:

async def get_rendered_template(
        template_filename: str, parameters: dict[str, Any], jinja_loader: BaseLoader
) -> str:
    """Render the given Template.

    :param template_filename: name of the template file
    :param parameters: Parameters passed to the template
    :param jinja_loader: Loader to find templates path, see https://jinja.palletsprojects.com/en/2.11.x/api/#loaders
    :return: Returns the rendered Jinja2 Template, which is a valid LaTex file
    """
    ...

This is what it looks like after running docformatter:

async def get_rendered_template(
        template_filename: str, parameters: dict[str, Any], jinja_loader: BaseLoader
) -> str:
    """Render the given Template.

    :param template_filename: name of the template file :param
    parameters: Parameters passed to the template :param jinja_loader:
    Loader to find templates path, see
    https://jinja.palletsprojects.com/en/2.11.x/api/#loaders
    :param template_filename: name of the template file
    :param parameters: Parameters passed to the template
    :param jinja_loader: Loader to find templates path, see
        https://jinja.palletsprojects.com/en/2.11.x/api/#loaders
    :return: Returns the rendered Jinja2 Template, which is a valid
        LaTex file
    """
    ...

Docformatter is running via pre-commit with this config:

  - repo: https://github.com/myint/docformatter
    rev: v1.7.5
    hooks:
      - id: docformatter
        args: ["--in-place", "--wrap-summaries", "100"]

fin swimmer

@github-actions github-actions bot added the fresh This is a new issue label Aug 15, 2023
@weibullguy weibullguy added P: bug PEP 257 violation or existing functionality that doesn't work as documented C: style Relates to docstring format style (e.g., Google, NumPy, Sphinx) and removed fresh This is a new issue labels Aug 15, 2023
@github-actions github-actions bot added the U: medium A relatively medium urgency issue label Aug 15, 2023
@huonw
Copy link

huonw commented Feb 7, 2024

Another example:

def f() -> None:
    """xx.

    :param a: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb s3://cccc.
    """

Running docformatter 1.7.5 with default settings on that file gives this diff:

--- before//tmp/docformatter-duplicate.py
+++ after//tmp/docformatter-duplicate.py
@@ -1,5 +1,8 @@
 def f() -> None:
     """xx.
 
-    :param b: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb s3://cccc.
+    :param b: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
+    s3://cccc.
+    :param b: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
+        s3://cccc.
     """

Things that seem to have to be true or this doesn't reproduce:

  • first line has to have at least two characters, i.e. x. alone stops the duplicate
  • the text for the parameter has to be long enough for it to split across multiple lines, e.g. when it is :param a: bb s3://cc it fits on one line and there's no duplicate
  • the URL has to have a "known" URL scheme, e.g. using yy:// stops the duplicate, but http:// has the duplicate

@lilatomic
Copy link

Using a git bisect, it looks like this started in cb7bf8d

Digging into it, I think I see what's going on:

  1. the xx. is necessary so the docstring is interpreted as having both a summary and a description, causing do_split_description to run
  2. a URL is identified. It is not identified as overlapping the field
  3. the URL causes the description to be wrapped ref. The description includes the field declaration. The wrapped lines are added to the buffer.
  4. while wrapping the field, the original description is used. The wrapped field is added to the buffer. (but since the lines were already added in the buffer, there are now duplicates)

The problem arises in step 2. There's a check to exclude urls that occur inside fields. But it doesn't exclude this URL. I think that's an expectation mismatch with the return of do_find_field_lists: that function return the indexes for just the param name (:param b:), and not the whole field definition which includes the URL. The function _do_join_field_body handles this by deciding that the body of the param either extends to the start of the next param, or until the end of the docstring.

I think that means that we could, in theory, simplify _field_over_url to reject any url with a start (or end, but I think that's covered) after the start of the first param. Doing that seems to pass all test cases (it splits the line and puts the url on a new line inset for the parameter, as expected).

@lilatomic lilatomic linked a pull request Jun 30, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: style Relates to docstring format style (e.g., Google, NumPy, Sphinx) P: bug PEP 257 violation or existing functionality that doesn't work as documented U: medium A relatively medium urgency issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants