Add option to format compatible with black: by NickHilton · Pull Request #166 · PyCQA/docformatter

NickHilton · 2023-03-02T21:11:00Z

Summary

Attempts to address the issue in #94 by adding a CLI arg to allow compatibility with black. This does not implement everything (i.e. pyproject.toml specifications) as I wanted feedback before continuing with this to finish the PR.

This adds a --black argument to the CLI args which, when passed, ensure that the issue from #94 is addressed. I'm not aware of any further discrepancies with black which need to be addressed but please let me know if there are

Updated
I also added a --black-line-length arg, only available if you specify --black which will automatically set --wrap-summaries and --wrap-descriptions to the value specified whilst defaulting to 88, consistent with black. I wasn't sure this was strictly necessary as the changes would be compatible with black even setting custom line lengths, however this was raised in the original issue as desired

Per comments - have left default behaviour of line length as 88 when --black is specified, otherwise use the current PEP8 defaults, and allow the user to specify wrap-summaries and wrap-descriptions as desired to overwrite this.

I added a unittest and tested the full script passing in the --black argument and got an expected result

Changes

Adds CLI arg --black to respect black formatting rules
Adds CLI arg --black-line-length to format line lengths consistently and with the black default
Updates _do_format_docstring to respect black formatting rules w.r.t. leading "
Adds unittest for _do_format_docstring for the black case
Updates docs

docs/source/requirements.rst

src/docformatter/configuration.py

tests/test_format_docstring.py

coveralls · 2023-03-03T19:17:02Z

Pull Request Test Coverage Report for Build 4326573190

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at ?%

Totals
Change from base Build 4326535638:	0%
Covered Lines:
Relevant Lines:	0

💛 - Coveralls

coveralls · 2023-03-03T19:17:09Z

Pull Request Test Coverage Report for Build 4326573190

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 0.0%

Totals
Change from base Build 4326535638:	0.0%
Covered Lines:	0
Relevant Lines:	0

💛 - Coveralls

s-weigand · 2023-03-04T13:49:13Z

Another necessary change for docformatter to play nicely with black is that _do_remove_blank_lines_after_method does not get executed (see #161 )

As for configuration, it would be nice if a config like this would work. Or line-length would get extracted from the black config section.

[tool.black]
line-length = 99

[tool.docformatter]
black = true
wrap-summaries = 99
wrap-descriptions = 99

Sorry for the unasked 2 cents, but docformatter and black fighting about formatting when running pre-commit is the reason why I'm sticking with docformatter==1.5.0 right now 😅

weibullguy · 2023-03-05T16:35:35Z

@s-weigand I think you'll see from my comments, that's exactly the way I'd like docformatter to work. As far as using options for another tool to modify docformatter's behavior, I'm not 100% sold on that. But, I'm not 100% against it either. As I stated in my review comments above,

The only thing --black NEEDS to modify is the --pre-summary-space behavior because docformatter already has arguments for line length.

Adding two lines, one time to a configuration file doesn't seem overly burdensome to me (of course, I walked up hill both ways to school when I was a kid, so....). This keeps the tools configurations separate which is what I prefer until convinced otherwise.

weibullguy · 2023-03-05T16:44:19Z

@NickHilton @s-weigand pointed out a behavior that needs to be modified by the --black argument. In format.py the Formatter._format_code method has the line:

modified_tokens = self._do_remove_blank_lines_after_method(
    modified_tokens
)

this should not be executed when the --black argument is passed. See #161.

NickHilton · 2023-03-06T15:48:09Z

I have addressed the blank lines after method issue:

When black runs it strips blank lines after method docstrings up to 1 blank line.

I wanted to replicate that behaviour rather than not format the blank lines at all after method docstrings:

To be consistent with black
To be consistent with the other behaviour of docformatter which includes formatting blank lines around the docstring as inside the scope of what docformatter will change

docs/source/usage.rst

docs/source/requirements.rst

tests/test_format_docstring.py

weibullguy

I presume the tests are passing on your end? For some reason they're not on GH. Also, there are some pycodestyle errors that need addressing.

NickHilton · 2023-03-15T13:00:12Z

I presume the tests are passing on your end? For some reason they're not on GH. Also, there are some pycodestyle errors that need addressing.

The one thats failing does... Although I can't get all the tests to pass on master branch so probably have something odd with my local set up. Will see if I can get it fixed

Will address the pycodestyle errors

weibullguy

I plan to release v1.6.0 this weekend which handles URLs. My plan is to then merge this PR and tag as v1.7.0-rc1. If retaining the blank lines in empty functions is going to be a heavy lift, I'd propose we NOT include it in this PR.

weibullguy · 2023-04-04T01:45:54Z

@NickHilton I just released v1.6.0. You'll want to merge those changes and make sure everything is still working.

- Adds CLI arg --black to respect black formatting rules - Adds CLI arg --black-line-length to format line lengths consistently and with the black default - Updates _do_format_docstring to respect black formatting rules w.r.t. leading " - Adds unittest for _do_format_docstring for the black case - Updates docs

NickHilton · 2023-04-13T12:20:27Z

Sorry for the delay - Have rebased but I need to edit a few things to fix the tests, will do so asap and then we can reassess which version it will roll out in

weibullguy requested changes Mar 3, 2023

View reviewed changes

docs/source/requirements.rst Outdated Show resolved Hide resolved

src/docformatter/configuration.py Outdated Show resolved Hide resolved

tests/test_format_docstring.py Show resolved Hide resolved

weibullguy added P: enhancement Feature that is outside the scope of PEP 257 C: style Relates to docstring format style (e.g., Google, NumPy, Sphinx) labels Mar 3, 2023

weibullguy mentioned this pull request Mar 3, 2023

Documentation refers to options that do not exist in the latest released version #167

Closed

NickHilton commented Mar 6, 2023

View reviewed changes

docs/source/usage.rst Show resolved Hide resolved

NickHilton requested a review from weibullguy March 6, 2023 16:07

weibullguy requested changes Mar 6, 2023

View reviewed changes

docs/source/requirements.rst Show resolved Hide resolved

tests/test_format_docstring.py Show resolved Hide resolved

NickHilton requested a review from weibullguy March 14, 2023 19:45

weibullguy reviewed Mar 14, 2023

View reviewed changes

weibullguy mentioned this pull request Mar 20, 2023

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

Closed

weibullguy reviewed Mar 20, 2023

View reviewed changes

weibullguy approved these changes Mar 20, 2023

View reviewed changes

zaneselvans mentioned this pull request Apr 10, 2023

Update docformatter requirement from <1.6,>=1.5 to >=1.5,<1.7 catalyst-cooperative/pudl#2509

Closed

Nick Hilton added 9 commits April 13, 2023 11:08

Update test comment

ad0e690

Add --black-line-length to docs

e814948

Remove additional black-line-length arg

7905d41

Match black format for blank lines after method docstring

c31fc9e

Blank line in documentation

9599f3f

Add other method type to test case

17319b9

Add a test

d683e49

Another test

0928849

Nick Hilton added 2 commits April 13, 2023 11:13

Update tests and lint code

52bfb88

clean up and another tets

919699d

NickHilton force-pushed the black-format-option branch from da9e9e1 to 919699d Compare April 13, 2023 10:14

weibullguy mentioned this pull request Apr 28, 2023

feat: add option to format compatible with black #196

Merged

weibullguy closed this in #196 Apr 28, 2023

weibullguy added the S: duplicate Closed as a duplicate issue or PR label Apr 28, 2023

Conversation

NickHilton commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coveralls commented Mar 3, 2023

Pull Request Test Coverage Report for Build 4326573190

💛 - Coveralls

Uh oh!

coveralls commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 4326573190

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

s-weigand commented Mar 4, 2023

Uh oh!

weibullguy commented Mar 5, 2023

Uh oh!

weibullguy commented Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NickHilton commented Mar 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weibullguy left a comment

Choose a reason for hiding this comment

Uh oh!

NickHilton commented Mar 15, 2023

Uh oh!

weibullguy left a comment

Choose a reason for hiding this comment

Uh oh!

weibullguy commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NickHilton commented Apr 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NickHilton commented Mar 2, 2023 •

edited

Loading

coveralls commented Mar 3, 2023 •

edited

Loading

weibullguy commented Mar 5, 2023 •

edited

Loading

weibullguy commented Apr 4, 2023 •

edited

Loading