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

Add argument to tell docformatter to be compatible with black #94

Closed
weibullguy opened this issue Aug 6, 2022 · 15 comments · Fixed by #196
Closed

Add argument to tell docformatter to be compatible with black #94

weibullguy opened this issue Aug 6, 2022 · 15 comments · Fixed by #196
Labels
C: style Relates to docstring format style (e.g., Google, NumPy, Sphinx) P: enhancement Feature that is outside the scope of PEP 257
Milestone

Comments

@weibullguy
Copy link
Member

As pointed out in #39, black inserts a space before a one-line or the summary in a multi-line docstring when it begins with a quote (") character. Default behavior for docformatter is to remove this space, which leads to a never-ending loop. The newly created argument --pre-summary-space will retain this black-inserted space at the expense of adding a space before every docstring, which black will then remove resulting in, yet again, a never-ending loop.

Add an argument (e.g., --black) to make docformatter only add the space when the line begins with a quote character when the --pre-summary-space argument is set. Thus, the invocation for black compatibility would be:

docformatter --pre-summary-space --black
@weibullguy weibullguy added the P: enhancement Feature that is outside the scope of PEP 257 label Aug 6, 2022
@weibullguy weibullguy changed the title feat: Add Argument to Tell docformatter to be Compatible with black Add Argument to Tell docformatter to be Compatible with black Aug 6, 2022
@weibullguy weibullguy changed the title Add Argument to Tell docformatter to be Compatible with black Add argument to tell docformatter to be compatible with black Aug 6, 2022
@weibullguy weibullguy added the C: style Relates to docstring format style (e.g., Google, NumPy, Sphinx) label Aug 6, 2022
@ichard26
Copy link

Other than PEP 257, we occasionally move quotes. I'm not entirely sure when Black does move quotes, but it's pretty rare and it's probably more an edge case than codified behaviour. For more see psf/black#1632 and psf/black#3044.

For posterity, here what the documentation has to say on docstrings:

Black also processes docstrings. Firstly the indentation of docstrings is corrected for both quotations and the text within, although relative indentation in the text is preserved. Superfluous trailing whitespace on each line and unnecessary new lines at the end of the docstring are removed. All leading tabs are converted to spaces, but tabs inside text are preserved. Whitespace leading and trailing one-line docstrings is removed.

https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#strings

cc @JelleZijlstra am I forgetting another way we format docstrings?

@thejcannon
Copy link

Should this be closed by #46?

@kdeldycke
Copy link

Maybe we can start by adding an entry for docformatter in Black integration documentation: https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html

Now I would love to have a dedicated --black option to handle all the details. In the mean time, my own set of options for compatibility is:

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

As for the never-ending loop, I addressed it by running docformatter before black in a single GitHub Action job: https://github.com/kdeldycke/workflows/blob/f7dd058dd0281f31d04c2192b277f1d6fb727677/.github/workflows/autofix.yaml#L120-L132

@weibullguy
Copy link
Member Author

@thejcannon I don't think so. #46 adds a space before the summary regardless of what the first character in the summary is. Black only adds the space if the first character is a double quote ". The plan for the --black option is to tell docformatter you only want a pre-summary space when the first character is a double quote (maybe if it's a single or double quote, I'm open to feedback).

Currently --pre-summary-space adds the space regardless of the first character. The --pre-summary-space option requested in #39 was for the existing behavior. The --black flag (not the '80's band) represents a new use case.

@kdeldycke here is a snippet from my docformatter "road map" notes:

Passing --black should set the following:

    --pre-summary-space (only if docstring begins with ")
    --wrap-descriptions 88
    --wrap-summaries 88

Or the pyproject.toml equivalent:

    [tool.docformatter]
    pre-summary-space = true
    wrap-descriptions = 88
    wrap-summaries = 88

@kdeldycke
Copy link

Thanks @weibullguy for comparing notes about the details of black-compatible options. You just confirmed my initial findings! :)

@danielhoherd
Copy link

Having just run into the incompatibility between black and docformatter 1.6, I'm really happy to see the work being done here. The suggestions that @weibullguy added are great, but I would also like to point out that black has configurable line length and other options. Should a black compatible docformatter inherit those black config options from the pyproject.toml, or should the user need to manually match the config options?

@NickHilton
Copy link

Put an attempt to address this issue above, hoping to get feedback on if its on the right track before finishing it

@weibullguy
Copy link
Member Author

weibullguy commented Mar 3, 2023

@danielhoherd

Should a black compatible docformatter inherit those black config options from the pyproject.toml, or should the user need to manually match the config options?

Other than line length, what other black options would docformatter need to accommodate?

@weibullguy
Copy link
Member Author

@NickHilton

Put an attempt to address this issue above, hoping to get feedback on if its on the right track before finishing it

I've provided some feedback in the PR.

@danielhoherd
Copy link

@weibullguy Line length is the only thing I know of that is problematic.

@gerbenoostra
Copy link

For the latest version (v1.6.0-rc5), docformatter removes whitelines after block comments.
For example, the following is how black formats the code:

class MyClass:
    def do_stuff(self):
        """Does stuff."""

    def do_more(self):
        """Does more."""


def next_method():
    return 1

After applying Docformatter:

class MyClass:
    def do_stuff(self):
        """Does stuff."""
    def do_more(self):
        """Does more."""
def next_method():
    return 1

DocFormatter version v1.5.1 seems to be the latest version that keeps the empty lines, keeping it compatible with black.

@weibullguy
Copy link
Member Author

@gerbenoostra that is correct. Per PEP-257, There’s no blank line either before or after the docstring. Put a pass or any other statement in each method and the blank line won't be stripped.

@gerbenoostra
Copy link

gerbenoostra commented Mar 20, 2023

I see I have another tool interfering. I also use autoflake (mainly to remove unused imports). And that removes the pass lines
But the root problem is Black not adhering to all PEP's, at least not PEP-257.

@weibullguy
Copy link
Member Author

weibullguy commented Mar 20, 2023

@NickHilton take a look at the black behavior leaving the blank lines in empty functions/methods. Try to include it in #166. I could also be convinced to add a new argument (e.g., --retain-blank-lines) for docformatter to retain blank lines in empty functions/methods and then have the --black argument set this true. See also #161.

@NickHilton
Copy link

Will take a look - Just pushed the other changes for #166 which I'm hoping make all the unittests pass for the behaviour other than this blank line thing.

Will hopefully get to look at that by Wednesday

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: enhancement Feature that is outside the scope of PEP 257
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants