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

Remove whitespaces of whitespace-only files #3348

Merged
merged 7 commits into from
Nov 12, 2022
Merged

Conversation

aaossa
Copy link
Contributor

@aaossa aaossa commented Oct 21, 2022

Description

Closes #2484
Closes #2382

Currently, empty and whitespace-only (with or without newlines) are not modified. In some discussions (issues and pull requests) consesus was to reformat whitespace-only files to empty or single-character files, preserving line endings when possible. With that said, this PR introduces the following behaviors:

  • Empty files are left as is
  • Whitespace-only files (no newline) reformat into empty files
  • Whitespace-only files (1 or more newlines) reformat into a single newline character

To implement these changes, we moved the initial check at format_file_contents that raises NothingChanged if the source (with no whitespaces) is an empty string. In the case of *.ipynb files, format_ipynb_string checks a similar condition and removed whitespaces. In the case of Python files, format_str_once includes a check on the output that returns the correct newline character if possible or an empty string otherwise.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

Currently, empty and whitespace-only (with or without newlines) are
not modified. In some discussions (issues and pull requests) consesus
was to reformat whitespace-only files to empty or single-character
files, preserving line endings when possible. With that said, this
commit introduces the following behaviors:

* Empty files are left as is
* Whitespace-only files (no newline) reformat into empty files
* Whitespace-only files (1 or more newlines) reformat into a single
newline character

To implement these changes, we moved the initial check at
`format_file_contents` that raises `NothingChanged` if the source
(with no whitespaces) is an empty string. In the case of *.ipynb
files, `format_ipynb_string` checks a similar condition and removed
whitespaces. In the case of Python files, `format_str_once` includes a
check on the output that returns the correct newline character if
possible or an empty string otherwise.

Signed-off-by: Antonio Ossa Guerra <[email protected]>
@github-actions
Copy link

github-actions bot commented Oct 21, 2022

diff-shades reports zero changes comparing this PR (58739ae) to main (c23a5c1).


What is this? | Workflow run | diff-shades documentation

tests/test_black.py Outdated Show resolved Hide resolved
@aaossa aaossa force-pushed the redo-pr-2484 branch 2 times, most recently from 4f704bd to 1a409b2 Compare October 24, 2022 19:44
This commit introduces two tests and modifies other two tests. The
introduced tests verify the expected behaviour on files containing a
single newline character via 1) `format_str` and 2)
`format_file_in_place`. In the other hand, the first modified test
(`test_format_file_contents`) verify that `NothingChanged` is raised
on files containing only a single newline, and that whitespace-only
files are properly formatted to a single newline character. The second
modified test (`test_reformat_one_with_stdin_empty`) validates the
expected behavior when the input is passed via stdin.

Before the fix introduced in the previous commit, these tests (and a
couple of others) failed on cases covering whitespace-only files. Now,
these tests are passed for all cases.

Signed-off-by: Antonio Ossa Guerra <[email protected]>
Add entry about new behavior on whitespace-only files: removing
whitespace characters and return a single newline (if present) or an
empty file

Signed-off-by: Antonio Ossa Guerra <[email protected]>
@aaossa aaossa marked this pull request as ready for review October 24, 2022 20:06
@ichard26
Copy link
Collaborator

Hey!

I just wanted to say thank you for submitting a PR. I know us maintainers haven't been very active reviewing PRs lately and that's disheartening. I speak for the rest of the maintainership team that we really appreciate all of your work.

I promise I've seen your PRs (this one, #3338 and #3336) and care about them. I've been really busy lately and finding time (and the energy) to do reviews has been tough. I'll try my best to find time this week.

Thanks again.

@aaossa
Copy link
Contributor Author

aaossa commented Oct 25, 2022

Hey @ichard26, I understand that maintaining such an important project for many people requires many hours, so no worries. I must say thank you to all the maintainership team for your work. I'll try to keep doing my part by contributing when I have the time (but no more PRs for now, don't worry 😅). Have a great week!

@JelleZijlstra
Copy link
Collaborator

Could you take a look at the merge conflict?

CHANGES.md Outdated
@@ -29,6 +29,8 @@
- Parsing support has been added for walruses inside generator expression that are
passed as function args (for example,
`any(match := my_re.match(text) for text in texts)`) (#3327).
- Reformat empty and whitespace-only files as either an empty file (if no newline is
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to go into the preview style (not just in the changelog, but also in the code).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be added in the future style page too?

@aaossa aaossa marked this pull request as draft October 26, 2022 17:23
@aaossa aaossa marked this pull request as ready for review October 26, 2022 19:31
@aaossa
Copy link
Contributor Author

aaossa commented Oct 26, 2022

This PR is now ready for review: conflicts with main were solved, and the changes were moved into the preview style 👌

@aaossa
Copy link
Contributor Author

aaossa commented Nov 10, 2022

Any updates on this PR? Seems to be ready and is already approved

Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

Looks fine in general, I don't like how poorly the tests read, but that's more a symptom of the poor test setup and infrastructure (ie. it's not your fault).

CHANGES.md Outdated Show resolved Hide resolved
src/black/__init__.py Show resolved Hide resolved
tests/test_black.py Outdated Show resolved Hide resolved
The new criteria to reformat empty and whitespace-only files should go
into the preview style

Signed-off-by: Antonio Ossa Guerra <[email protected]>
Adding comments on a non-intuitive behavior ar `_format_str_once`, and
improving the `test_one_empty_line` test readability by using a
pre-defined function that implements the same assertions

Signed-off-by: Antonio Ossa Guerra <[email protected]>
@aaossa aaossa requested a review from ichard26 November 11, 2022 13:15
Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@ichard26 ichard26 merged commit d97b789 into psf:main Nov 12, 2022
@aaossa aaossa deleted the redo-pr-2484 branch November 12, 2022 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All-whitespace files do not get their newlines removed
3 participants