Code Flag Options #2259

HassanAbouelela · 2021-05-26T02:45:29Z

Closes #2104, closes #1801.

Fixes a bug in the --code flag which would ignore options such as --diff, --check, and --fast.

This PR implements the fix described in the issue. It also adds tests for this fix.

Properly handles the diff, color, and fast option when black is run with the `--code` option. Signed-off-by: Hassan Abouelela <[email protected]>

Adds test for the CLI `--code` option. Signed-off-by: Hassan Abouelela <[email protected]>

Updates the docs to remove the warning about the code options bug, and adds a changelog entry. Signed-off-by: Hassan Abouelela <[email protected]>

HassanAbouelela · 2021-05-26T12:15:31Z

I've fixed the linting issues now, could someone rerun the workflows.

cooperlees

Wow, great tests here! Thanks for your contribution.

Signed-off-by: Hassan Abouelela <[email protected]>

ichard26

Thanks for working on this fix! I got some feedback but nothing truly blocking (except for possibly a Windows bug).

Great tests btw!

src/black/__init__.py

tests/test_black.py

src/black/__init__.py

Wraps the output stream for windows to fix the color option when using diff. Signed-off-by: Hassan Abouelela <[email protected]>

Switches to a plain assert instead of unittest's self.assertEqual in the `--code` option tests. Signed-off-by: Hassan Abouelela <[email protected]>

ichard26

This time I did a hands-on review / test and this still isn't quite right. The error message when formatting fails isn't right. For normal cases, this is handled by the Report class. Check out the difference between these cases:

--code:

STDIN:

It honestly might be better to generalize reformat_one to handle the --code case too.

Thanks for being patient with me!

tests/test_black.py

src/black/__init__.py

HassanAbouelela · 2021-05-27T23:10:28Z

Thanks for the feedback, and I totally agree. Originally I wanted to maintain the whole “thrown together, mainly for testing” behavior, but as the PR has progressed, the feature has become quite bloated, and repeated a lot of the same code from the other functions. I’ll see what I can do to formalize this flag a bit more.

Switches the code option to use the STDIN formatter instead of manually formatting the input, to add support for other flags, and properly handle the output. Signed-off-by: Hassan Abouelela <[email protected]>

HassanAbouelela · 2021-05-28T22:11:51Z

Updated the code to use the STDIN formatter.

As a note for maintainers: we should probably squash/rebase this PR before merging. Most of the commits prior to switching to the STDIN formatter are irrelevant.

JelleZijlstra · 2021-05-28T22:18:26Z

We always squash merge, fyi. Also, could you fix the merge conflict in CHANGES.md?

# Conflicts: # CHANGES.md

HassanAbouelela · 2021-05-28T22:31:09Z

Thanks, good to know. The conflicts have been fixed.

JelleZijlstra

Thanks, this looks significantly cleaner

src/black/__init__.py

src/black/report.py

JelleZijlstra · 2021-05-29T14:53:08Z

Just tried this out locally and decided that it's better to make -c apply --quiet by default because the extra "All done" output isn't really useful.

There might also be a discrepancy regarding whether pyproject.toml is used; looking into that more now.

JelleZijlstra · 2021-05-29T15:09:25Z

OK, there's indeed a problem with pyproject.toml:

(black) jelle@mbpt-root tmp % black -c "a = 'b'"  
a = "b"
(black) jelle@mbpt-root tmp % echo "a = 'b'" | black -
a = 'b'
All done! ✨ 🍰 ✨
1 file left unchanged.
(black) jelle@mbpt-root tmp % cat pyproject.toml 
[tool.black]
skip-string-normalization = true

I think the correct behavior is to pick up the pyproject.toml. That way, users can run black -c in their project directory to see how black would format code in that project.

The reason for the discrepancy is that in black.read_pyproject_toml, we pass () to black.files.find_pyproject_toml, and that function returns Path("/").resolve() if no src is provided. I think it should instead default to searching for a toml in the cwd. The current behavior would make it pick up /pyproject.toml at the file system root, which doesn't make much sense.

Signed-off-by: Hassan Abouelela <[email protected]>

Changes the find project root function to look in the current directory instead of system root when no sources are passed. Signed-off-by: Hassan Abouelela <[email protected]>

src/black/files.py

Changes the find project root function to look in the current directory instead of system root when no sources are passed. Signed-off-by: Hassan Abouelela <[email protected]>

Changes the assertions made on mock calls to work with python version earlier than 3.8. Signed-off-by: Hassan Abouelela <[email protected]>

ichard26

I am happy with the final product for the most part. The only thing I'd dock against this patch is the general awkwardness of using format_stdin_to_stdout in reformat_code, but this is already quite complex so I'd rather refactor this (and other parts of the "main called to formatting will start" pathway) in a separate PR.

Thank you so much for your contribution! This project is only possible by contributions like these 🖤. You're awesome, @HassanAbouelela! Congratulations on your first PR for psf/black. It's great to be able to get rid of that documentation warning for once and all. Also thanks for playing a role in the very nice welcome to Python Discord.

P.S. since you seem to be *the* person who manages to break all of our development workflows, it would be even more awesome if you could provide some feedback on your experience contributing here, we got a lot to learn (especially in the Windows department) so yeah. More details in #2238.

cooperlees · 2021-06-02T02:42:16Z

This needs the CHNAGES.md fixed ... Wonder how we can automate checking for that ...

ichard26 · 2021-06-02T02:44:28Z

Oh fun! Probably automatiable although that will make it harder to cleanly separate the logic into own repo when I eventually do that.

Good catch!

…S.md

cooperlees · 2021-06-02T02:46:34Z

5de8c5f - Just pushed it.

HassanAbouelela added 3 commits May 26, 2021 05:32

Adds Option Logic For Inline Code

2bc78b3

Properly handles the diff, color, and fast option when black is run with the `--code` option. Signed-off-by: Hassan Abouelela <[email protected]>

Adds Code Option Tests

7bb9937

Adds test for the CLI `--code` option. Signed-off-by: Hassan Abouelela <[email protected]>

Updates Docs With Code Options

1d0faae

Updates the docs to remove the warning about the code options bug, and adds a changelog entry. Signed-off-by: Hassan Abouelela <[email protected]>

HassanAbouelela force-pushed the code-options branch from 6fb6ca1 to 1d0faae Compare May 26, 2021 12:14

Merge branch 'main' into code-options

81ba0f6

cooperlees approved these changes May 26, 2021

View reviewed changes

Adds Missing CHANGE

7b8f6d2

Signed-off-by: Hassan Abouelela <[email protected]>

ichard26 reviewed May 26, 2021

View reviewed changes

src/black/__init__.py Outdated Show resolved Hide resolved

src/black/__init__.py Outdated Show resolved Hide resolved

tests/test_black.py Outdated Show resolved Hide resolved

src/black/__init__.py Outdated Show resolved Hide resolved

HassanAbouelela added 2 commits May 27, 2021 02:53

Fixes Code Option Color On Windows

6221b88

Wraps the output stream for windows to fix the color option when using diff. Signed-off-by: Hassan Abouelela <[email protected]>

Switches To Assert Format In Code Tests

081a8e3

Switches to a plain assert instead of unittest's self.assertEqual in the `--code` option tests. Signed-off-by: Hassan Abouelela <[email protected]>

HassanAbouelela requested a review from ichard26 May 26, 2021 23:58

ichard26 requested changes May 27, 2021

View reviewed changes

tests/test_black.py Show resolved Hide resolved

ichard26 reviewed May 27, 2021

View reviewed changes

src/black/__init__.py Outdated Show resolved Hide resolved

Uses STDIN Formatter For Code Option

eefcd2e

Switches the code option to use the STDIN formatter instead of manually formatting the input, to add support for other flags, and properly handle the output. Signed-off-by: Hassan Abouelela <[email protected]>

HassanAbouelela requested a review from ichard26 May 28, 2021 22:12

Merge branch 'main' into code-options

9221268

# Conflicts: # CHANGES.md

JelleZijlstra requested changes May 29, 2021

View reviewed changes

src/black/__init__.py Outdated Show resolved Hide resolved

src/black/report.py Outdated Show resolved Hide resolved

JelleZijlstra added 2 commits May 29, 2021 07:35

Use a fake Path

39c3ae1

make -c quiet by default

5f85ff5

HassanAbouelela added 3 commits May 29, 2021 20:10

Remove Explicit Quiet Flags In Code Tests

a6a40ce

Signed-off-by: Hassan Abouelela <[email protected]>

Merge branch 'main' into code-options

52f9543

Searches Current Directory When No Config Is Passed

37ff539

Changes the find project root function to look in the current directory instead of system root when no sources are passed. Signed-off-by: Hassan Abouelela <[email protected]>

JelleZijlstra reviewed May 29, 2021

View reviewed changes

src/black/files.py Outdated Show resolved Hide resolved

HassanAbouelela added 2 commits May 29, 2021 22:28

Searches Current Directory When No Config Is Passed

81b2458

Changes the find project root function to look in the current directory instead of system root when no sources are passed. Signed-off-by: Hassan Abouelela <[email protected]>

Makes Mock Call Assertions Compatible With <py3.8

adaa41e

Changes the assertions made on mock calls to work with python version earlier than 3.8. Signed-off-by: Hassan Abouelela <[email protected]>

JelleZijlstra approved these changes Jun 2, 2021

View reviewed changes

ichard26 approved these changes Jun 2, 2021

View reviewed changes

JelleZijlstra merged commit 7567cdf into psf:main Jun 2, 2021

HassanAbouelela deleted the code-options branch June 2, 2021 01:55

HassanAbouelela mentioned this pull request Jun 2, 2021

Improving the contributing experience #2238

Open

cooperlees added a commit that referenced this pull request Jun 2, 2021

Move --code #2259 change log to correct unlreased section of CHANGE…

5de8c5f

…S.md

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Flag Options #2259

Code Flag Options #2259

HassanAbouelela commented May 26, 2021 •

edited

Loading

HassanAbouelela commented May 26, 2021

cooperlees left a comment

ichard26 left a comment •

edited

Loading

ichard26 left a comment

HassanAbouelela commented May 27, 2021

HassanAbouelela commented May 28, 2021

JelleZijlstra commented May 28, 2021

HassanAbouelela commented May 28, 2021

JelleZijlstra left a comment

JelleZijlstra commented May 29, 2021

JelleZijlstra commented May 29, 2021

ichard26 left a comment

cooperlees commented Jun 2, 2021

ichard26 commented Jun 2, 2021 •

edited

Loading

cooperlees commented Jun 2, 2021 •

edited

Loading

Code Flag Options #2259

Code Flag Options #2259

Conversation

HassanAbouelela commented May 26, 2021 • edited Loading

HassanAbouelela commented May 26, 2021

cooperlees left a comment

Choose a reason for hiding this comment

ichard26 left a comment • edited Loading

Choose a reason for hiding this comment

ichard26 left a comment

Choose a reason for hiding this comment

HassanAbouelela commented May 27, 2021

HassanAbouelela commented May 28, 2021

JelleZijlstra commented May 28, 2021

HassanAbouelela commented May 28, 2021

JelleZijlstra left a comment

Choose a reason for hiding this comment

JelleZijlstra commented May 29, 2021

JelleZijlstra commented May 29, 2021

ichard26 left a comment

Choose a reason for hiding this comment

cooperlees commented Jun 2, 2021

ichard26 commented Jun 2, 2021 • edited Loading

cooperlees commented Jun 2, 2021 • edited Loading

HassanAbouelela commented May 26, 2021 •

edited

Loading

ichard26 left a comment •

edited

Loading

ichard26 commented Jun 2, 2021 •

edited

Loading

cooperlees commented Jun 2, 2021 •

edited

Loading