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

Empty and all-whitespace files become a single empty line and a linefeed #2484

Closed
wants to merge 5 commits into from

Conversation

akaihola
Copy link

@akaihola akaihola commented Sep 7, 2021

Description

Currently Python source files with only whitespace (space, tab, linefeed) as well as zero-length files are not reformatted but kept intact. In #2382 the consensus was that all of these should become a file with a single empty line, i.e. just a linefeed character.

Also, currently black.format_str() turns all-whitespace strings into zero-length strings, which is inconsistent compared to the command line and the black.format_file_contents() function. Wtih this patch, both functions reformat the source into a single newline.

This PR fixes issue #2382.

Note: See comments for the discussion on whether zero-length files should be kept intact, turned into a single LF, or turned into a single platform-dependent line terminator.

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

CHANGES.md Outdated Show resolved Hide resolved
@akaihola akaihola force-pushed the all-whitespace-files branch from 1111da2 to 4ded0f1 Compare September 7, 2021 19:42
Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

I love making this a common black practice.

I only have a question about if we come across windows empty text files - Should we maintain them or just say "black "\n's" all empty .py files etc. - I'm fine with the latter - Just want to see if others are ...

Comment on lines 1063 to 1064
if not dst_contents:
return "\n"
Copy link
Collaborator

@cooperlees cooperlees Sep 8, 2021

Choose a reason for hiding this comment

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

What do other formatters do here when running on windows vs. POSIX?

Should we (if found) leave Windows line terminators alone? (i.e. "\r\n")

Copy link
Collaborator

Choose a reason for hiding this comment

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

That'd be good for consistency with nonempty files 😅

Copy link
Author

@akaihola akaihola Sep 8, 2021

Choose a reason for hiding this comment

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

Should we (if found) leave Windows line terminators alone?

I believe this patch doesn't change Black's behavior with regard to existing line terminators. (not true, see below)

Replacing "\n" with os.linesep here and in corresponding tests should make this behave in a more native way on Windows, right?

Suggested change
if not dst_contents:
return "\n"
if not dst_contents:
return os.linesep

Copy link
Author

Choose a reason for hiding this comment

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

I believe this patch doesn't change Black's behavior with regard to existing line terminators.

Ah, but of course it does. A whitespace-only file with Windows line terminator first becomes empty, and is then changed into a single Unix linefeed. Hm.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah - This could also induce changes all the time if you have developers on Windows vs. POSIX ... i.e. file gets reformatted on Windows, then committed, then a POSIX dev will reformat and commit (we could have a constant battle)

We will just have to pick the one - which I think \n is more accepted in Python source files. Editors have been taught to leave this alone by default usually I think.

Copy link

@septatrix septatrix Sep 10, 2021

Choose a reason for hiding this comment

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

What do other formatters do here when running on windows vs. POSIX?

Should we (if found) leave Windows line terminators alone? (i.e. "\r\n")

I do not have a windows machine at my disposal but I tested a few formatters from different languages:

  • Yapf: Leaves a completely empty file as is, a whitespace only file without newline gets converted to \n, a file with a single newline (\n or \r\n) is preserved the way it was before and a file with multiple newlines is truncated to a \n (even if the newlines were \r\n).
  • Prettier (for JS): Defaults to \n for each platform.
  • Rustfmt (for Rust): Seems to convert an empty file to \n, otherwise identical to yapf.

Personally I think it would be the best compromise is to leave a completely empty file as is and a file with non-newline whitespace should be converted to a completely empty file. For all other files we can infer the line ending and preserve it.

I think it should be the system line ending, because git can be used for conversion. I don't really know if it is more common to still use \n, but at least I've not encountered it. Might have missed it though as well, if editors are transparent about that.

As nowadays nearly every editors supports both line endings it is increasingly common to checkout the repo with the original file ending (where \n is by far in the majority)

Copy link
Author

@akaihola akaihola Sep 10, 2021

Choose a reason for hiding this comment

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

So the "votes" so far for what zero-length files should become on Windows go like:

I'm counting half a vote for those who are ok with two different alternatives. @JelleZijlstra & @cooperlees, do you want to choose just one preference based on the discussion above? Is "voting" even a good way to reach a design decision in Black, or do we have a BDFL to dictate the choice?

Also, votes for files with only whitespace but no newlines:

  • 1 for converting to a zero-length file: @septatrix (in this issue)
  • 1 for keeping them intact: @zsol (in this issue)

Copy link
Author

@akaihola akaihola Sep 12, 2021

Choose a reason for hiding this comment

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

Found one linting issue with empty files:

$ echo >/tmp/empty.py
$ flake8 /tmp/empty.py
/tmp/empty.py:1:1: W391 blank line at end of file
$ pylint /tmp/empty.py      
************* Module empty
/tmp/empty.py:1:0: C0305: Trailing newlines (trailing-newlines)

This can actually be seen in action in the Lint step of the GitHub workflow of this very issue.

So Flake8 and Pylint seem to think empty source files should be zero-length instead of containing any line terminators! I added these linters as "voters" above.

Copy link
Collaborator

@zsol zsol Sep 12, 2021

Choose a reason for hiding this comment

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

I think

  • empty files should be left empty
  • files containing only whitespace but no newlines should be left alone

Copy link
Author

Choose a reason for hiding this comment

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

I now changed the implementation to emit a

  • zero-length file for empty or all-whitespace input but now line terminators
  • single line terminator for all-whitespace input with at least one line terminator, preserving LF/CRLF line terminators

@akaihola akaihola force-pushed the all-whitespace-files branch 2 times, most recently from 5d8727b to f793dc3 Compare September 9, 2021 18:28
@akaihola akaihola force-pushed the all-whitespace-files branch from f793dc3 to 06b4bd5 Compare September 10, 2021 12:07
Antti Kaihola added 3 commits September 11, 2021 19:55
We need to "surgically" make sure only the intended invocation of
`io.TextIOWrapper()` is patched, and other calls get through to the
original implementation.

See discussion in psf#2489
@akaihola akaihola force-pushed the all-whitespace-files branch from 06b4bd5 to 5806822 Compare September 11, 2021 17:08
@akaihola
Copy link
Author

I couldn't figure out why there were linting errors in the GitHub Actions workflow for files this branch didn't even touch.

@ichard26
Copy link
Collaborator

if it's flake8 related, it's a known issue, we unfortunately did not pin a plugin we use and now there's new warnings.

akaihola added a commit to akaihola/darker that referenced this pull request Sep 18, 2021
@akaihola
Copy link
Author

@ichard26 yes, I'm seeing Flake8 failures – interestingly different ones for different builds, some for files I didn't modify.

In addition, in the latest build, coverage can't be submitted:

Failed submitting coverage with service_name: github
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/coveralls/api.py", line 290, in submit_report
    response.raise_for_status()
  File "/usr/local/lib/python3.8/site-packages/requests/models.py", line 953, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 405 Client Error: Not Allowed for url: https://coveralls.io/api/v1/jobs

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/src/entrypoint.py", line 62, in run_coveralls
    result = coveralls.wear()
  File "/usr/local/lib/python3.8/site-packages/coveralls/api.py", line 257, in wear
    return self.submit_report(json_string)
  File "/usr/local/lib/python3.8/site-packages/coveralls/api.py", line 293, in submit_report
    raise CoverallsException(
coveralls.exception.CoverallsException: Could not submit coverage: 405 Client Error: Not Allowed for url: https://coveralls.io/api/v1/jobs
Trying submitting coverage with service_name: github-actions...
Patching os.environ with: {'COVERALLS_REPO_TOKEN': '***', 'COVERALLS_PARALLEL': 'true', 'COVERALLS_FLAG_NAME': 'py3.9-ubuntu-latest'}

The same happens twice, the second time with service_name: github-actions instead of github.

akaihola added a commit to akaihola/darker that referenced this pull request Sep 20, 2021
@akaihola akaihola mentioned this pull request May 5, 2022
@ichard26 ichard26 added the S: up for grabs (PR only) Available for anyone to work on as the PR author is busy or unreachable. label Aug 3, 2022
@akaihola akaihola deleted the all-whitespace-files branch November 13, 2022 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: up for grabs (PR only) Available for anyone to work on as the PR author is busy or unreachable.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants