Skip to content

Handle Docstrings as bytes + strip all whitespace#2037

Merged
JelleZijlstra merged 3 commits into
psf:masterfrom
CiderMan:python2-docstring-formatting-bugfix
Apr 22, 2021
Merged

Handle Docstrings as bytes + strip all whitespace#2037
JelleZijlstra merged 3 commits into
psf:masterfrom
CiderMan:python2-docstring-formatting-bugfix

Conversation

@CiderMan

@CiderMan CiderMan commented Mar 12, 2021

Copy link
Copy Markdown
Contributor

(Eventually) reproduced with a test and 'fixed'.

Successfully ran:

  • tox -e py
  • tox -e fuzz
  • black-primer

(fixes #1844, fixes #1923, fixes #1851, fixes #2002, fixes #2103)

@ichard26 ichard26 self-requested a review March 13, 2021 04:52
@cooperlees cooperlees changed the title Fix issue #1844 Handle Docstrings as bytes + strip all whitespace (fix #1844) Mar 29, 2021

@cooperlees cooperlees left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the code + test clearly showing this work.

Will see if I can add a changelog entry here.

@JelleZijlstra

Copy link
Copy Markdown
Collaborator

Looks like we still need a news entry here, and @ichard26 may still want to review. The code otherwise looks good to me.

@ichard26

Copy link
Copy Markdown
Collaborator

Yeah, to be clear, I do want to do a review just haven't been able to do one yet.

@ichard26 ichard26 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I don't quite understand the str vs bytes difference between Python 2 and 3 (that's why I haven't explicitly approved the PR), I don't see any issue with this change so consider me no longer a blocker to merging this PR.

Thanks for the time investment and work put into this, it helps us out a bunch!

@ichard26

Copy link
Copy Markdown
Collaborator

Oh, by the way, you can use special keywords in the PR description so GitHub will automatically close an issue upon merge. You can read more here: https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword. This makes sure any issue that should be closed are in fact closed (and I won't have to go proactively hunt for such issues!), keeping our very chaotic issue tracker ever so slightly less chaotic and scary :)

@CiderMan CiderMan changed the title Handle Docstrings as bytes + strip all whitespace (fix #1844) Handle Docstrings as bytes + strip all whitespace (fixes #1844, #1923, #1851, #2002) Apr 12, 2021
@CiderMan CiderMan changed the title Handle Docstrings as bytes + strip all whitespace (fixes #1844, #1923, #1851, #2002) Handle Docstrings as bytes + strip all whitespace (fixes #1844, fixes #1923, fixes #1851, fixes #2002) Apr 12, 2021
@ichard26 ichard26 self-requested a review April 13, 2021 03:39
@CiderMan CiderMan changed the title Handle Docstrings as bytes + strip all whitespace (fixes #1844, fixes #1923, fixes #1851, fixes #2002) Handle Docstrings as bytes + strip all whitespace (fixes #1844, fixes #1923, fixes #1851, fixes #2002, fixes #2103) Apr 22, 2021
@CiderMan

Copy link
Copy Markdown
Contributor Author

Hi Guys, Is there anything preventing this PR from being merged? The bug has been reported a number of times so it would be good to get the fix onto master ASAP.

On a related note, does anyone know when we might get a new version pushed to PyPI? We can't (currently) roll out a new version unless it is on PyPI and there are a couple of bug fixes that are on master that we would really like to get...

Cheers!

@JelleZijlstra

Copy link
Copy Markdown
Collaborator

Could you add a news entry (https://github.com/psf/black/blob/master/CONTRIBUTING.md#news--changelog-requirement)? Apart from that, I'd be happy to merge this.

@ichard26

ichard26 commented Apr 22, 2021

Copy link
Copy Markdown
Collaborator

To be clear, I want to rereview this before approving its merge. In terms of timeline, I should be able to get a review done around 10:40-ish AM (EST time), and if not, by the end of the day. Sorry for holding up the PR!

@ichard26 ichard26 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved! Other than adding a changelog entry (already asked by Jelle), this looks good to go.

By the way, linking issues to automatically close is done via the PR description, not title, so if you could fix that, that would be awesome!

My only concern is that any unintentional stripping of newlines in docstrings will go undetected, but that's what tests should be checking anyway :)

My apologies, I got this PR mixed up with GH-1740 and thought these changes would affect formatting output, but it doesn't ... oops.

Thanks for being patient with me and for working on this!

@CiderMan CiderMan changed the title Handle Docstrings as bytes + strip all whitespace (fixes #1844, fixes #1923, fixes #1851, fixes #2002, fixes #2103) Handle Docstrings as bytes + strip all whitespace Apr 22, 2021
@JelleZijlstra JelleZijlstra merged commit 5316bbf into psf:master Apr 22, 2021
@CiderMan CiderMan deleted the python2-docstring-formatting-bugfix branch April 22, 2021 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants