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

Enable enable-unstable-feature = ['string_processing'] without preview flag #4196

Open
matejsp opened this issue Jan 30, 2024 · 12 comments
Open
Labels
T: bug Something isn't working

Comments

@matejsp
Copy link

matejsp commented Jan 30, 2024

Describe the bug
In 2023 we used experimental-string-processing. This was enabled without preview style enabled.
However now in 2024 the experimental string processing flag was removed and replaced with
enable-unstable-feature = ['string_processing']

However this feature requires turning on the preview feature.
And now the files are additionally formated based on the preview style.

To Reproduce

preview = true
enable-unstable-feature = ['string_processing']
skip-string-normalization = true

vs:

experimental-string-processing = true
skip-string-normalization = true

We would like to go according to official style but have better string processing enabled. Without that we get bunch of line too long errors in the code.

For example, take this code (first without preview and the second with):
image

Expected behavior
I like that you are formalizing the enable-unstable-feature in such way but not that it requires preview with additional modifications. So unstable processing without other preview style (like in 2023).

Environment

  • Black's version: 24.1.1
  • OS and Python version: Python 3.11.0
@matejsp matejsp added the T: bug Something isn't working label Jan 30, 2024
@hauntsaninja
Copy link
Collaborator

Thanks for the issue! This was intentional. Our goal with --enable-unstable-feature was not to let users pick and choose parts of Black's future style (Black will simply never be configurable in that kind of way). It was to give Black a way to develop risky or problematic changes, while allowing us to potentially gain feedback and to allow users of such features a way to avoid thrash in their code in the event we mark a preview feature as unstable. See #4093 for more.

With that said, I'm sympathetic to the fact that despite being explicitly experimental --experimental-string-processing has a long history, and that this is a regression in the utility Black provides you. I'm still inclined to not make a change here, but I'm potentially willing to reconsider if it turns out that there are a lot of people eager to regain --experimental-string-processing but unwilling to use --preview. And of course, other maintainers may have other opinions.

@matejsp
Copy link
Author

matejsp commented Jan 30, 2024

IMHO Best way forward would be to make experimental string processing non experimental :D
The default config just doesn't work. One thing would be to leave correctly formated string but black just corrupts it (we get hundreds of line too longs warnings).

We have been using it for past year (on around 500k LOC) and it work without a problem (compatible with ruff, flake8, pylint). Without having (only) experimental processing latest black has became a bit useless for us :(

Also on larger code base it would be benefical to enable just some parts of preview features (in case they can be applied in isolation). It would be easier to test and not introduce too many changes in the code. The preview functionality gets in the end cherry picked anyway making it even better argument for such isolated preview features. Why not allow us to do the same during the development for next year style and be more engaged in the future black style :)

@JelleZijlstra
Copy link
Collaborator

One thing would be to leave correctly formated string but black just corrupts it (we get hundreds of line too longs warnings).

Could you provide more detail on this? It sounds like a bug and I don't think that's Black's normal behavior.

% cat longstring.py 
x = "a b c d d e f g h i j k l m n o p q r s t u v w x y z a b c d e f g h i j k l m n o p q r s t u v w x y z a b c d e f g h i j k l m n o p q r s t u v w x y z"
% black --unstable longstring.py 
reformatted longstring.py

All done! ✨ 🍰 ✨
1 file reformatted.
% cat longstring.py 
x = (
    "a b c d d e f g h i j k l m n o p q r s t u v w x y z a b c d e f g h i j k l m n"
    " o p q r s t u v w x y z a b c d e f g h i j k l m n o p q r s t u v w x y z"
)
% black longstring.py 
All done! ✨ 🍰 ✨
1 file left unchanged.
% cat longstring.py  
x = (
    "a b c d d e f g h i j k l m n o p q r s t u v w x y z a b c d e f g h i j k l m n"
    " o p q r s t u v w x y z a b c d e f g h i j k l m n o p q r s t u v w x y z"
)

The preview functionality gets in the end cherry picked anyway making it even better argument for such isolated preview features.

This is why we added the new --unstable style this year: we noticed that we often skipped out large parts of the preview style at the end of the year, making --preview not very useful as a preview of next year's style. This year, we put features we wouldn't currently stabilize in --unstable alone, so that --preview is much more likely to match next year's stable style.

IMHO Best way forward would be to make experimental string processing non experimental :D

If you want this to happen, there's a long list of issues related to it listed in #4042 that need to be fixed.

@matejsp
Copy link
Author

matejsp commented Jan 30, 2024

Sure.

So our configration is like this:

line-length = 120
target-version = ['py311']
preview = true
enable-unstable-feature = ['string_processing']

And then you have such code that is correctly formatted via experimental string processing (actually logger.info but I replaced it with print).

print(
    "[BATCH_USER_RESIDENCY_CHANGE] Migration completed in {0} seconds. Average batch duration was {1} seconds. User!"
    .format(5, 10, 10)
)

Now when you disable it ... (preview and enable-unstable-feature).

You get:

print(
    "[BATCH_USER_RESIDENCY_CHANGE] Migration completed in {0} seconds. Average batch duration was {1} seconds. User!".format(
        5, 10, 10
    )
)

And second line exceeds 120 chars making ruff, flake8 and other checkers complain.

black_issue.py:2:121: E501 Line too long (125 > 120)

And we have tons of logging.info() with long messages that are getting corrupted by this issue (for 2022-2024 black).

All the issues are related to string.format ... we use them also raise exceptions.SomeError(".. very long string {id} ... ".format(id=id, )

@matejsp
Copy link
Author

matejsp commented Feb 4, 2024

@JelleZijlstra did you manage to reproduce the issue that we are facing with current black offical string processing with format?

I just remembered that I posted this issue a year ago and it was just closed as that you don't want to change the style during the year and experimental processing will likely be stable in 2024:
#3622

For now we are sticking with 2023 version because 2024 in current state is unfournately broken.
I am surprised that this .format issue is not more common. I mean such a basic python functionality.

@JelleZijlstra
Copy link
Collaborator

I can confirm what you're seeing, but it only works if the line length matches up exactly:

% black -c '''print(
    "[BATCH_USER_RESIDENCY_CHANGE] Migration completed in {0} seconds. Average batch duration was {1} seconds. User!"
    .format(5, 10, 10)
)''' --unstable --line-length 110
print(
    "[BATCH_USER_RESIDENCY_CHANGE] Migration completed in {0} seconds. Average batch duration was {1}"
    " seconds. User!".format(5, 10, 10)
)
% black -c '''print(
    "[BATCH_USER_RESIDENCY_CHANGE] Migration completed in {0} seconds. Average batch duration was {1} seconds. User!"
    .format(5, 10, 10)
)''' --unstable --line-length 120
print(
    "[BATCH_USER_RESIDENCY_CHANGE] Migration completed in {0} seconds. Average batch duration was {1} seconds. User!"
    .format(5, 10, 10)
)
% black -c '''print(
    "[BATCH_USER_RESIDENCY_CHANGE] Migration completed in {0} seconds. Average batch duration was {1} seconds. User!"
    .format(5, 10, 10)
)''' --unstable --line-length 130
print(
    "[BATCH_USER_RESIDENCY_CHANGE] Migration completed in {0} seconds. Average batch duration was {1} seconds. User!".format(
        5, 10, 10
    )
)

I could see us make a change where we move .format( to a separate line, but we should do that in a separate preview feature.

Realistically I don't see us turning on string-processing by default. I opened #4208 about this.

@matejsp
Copy link
Author

matejsp commented Feb 5, 2024

Is that preview feature require whole preview enabled making it basically very hard to test and still not usable in our case.
What we have currently in preview if we are forced to enable are contraversal or/and buggy features leftovers missed from 2024 round of cherry picking.

Having preview style enabled one by one would be super useful.
I mean even with compatbility with other tools you relly on being able to turn each validation on/off. Imagine those tools having the same limitation as black.
https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html#configuration

For solving the bug under #4208 is more than just solving this concrete bug. I don't mind the proposed changes but I can see combining strings that are splitted in code would make less readable choice for some string being stuck under contraversal umbrealla for another year or two.

But still testing the impact in isolation coming back to first comment and this bug as a whole.

@JelleZijlstra
Copy link
Collaborator

What we have currently in preview if we are forced to enable are contraversal or/and buggy features leftovers missed from 2024 round of cherry picking.

That is not true. The current --preview features are mostly features that were added too late for the 2024 stable style, and the features we consciously rejected from the 2024 stable style are now in --unstable. (There is one feature that is in the current release's preview style that is currently slated to move to unstable for the next release.)

@matejsp
Copy link
Author

matejsp commented Feb 5, 2024

Sorry for missunderstanding but I saw 50% more changes in preview than going to 2024 (our numbers bellow). Nevertheless preview is still a moving target even if a bit more stable (than unstable). Every black we are in fear what will be broken again and feature set will stabilize only near end of the year.

Just to put the things into perspective (our perspective in huge monolit):

# version 23.12.1 experimental string processing
> black . --experimental-string-processing
All done! ✨ 🍰 ✨
14360 files left unchanged.

> ruff .
(no response means 0 errors)

# default 2023 style with disabled experimental string processing ... (introducing bunch of too long lines)
> black .
All done! ✨ 🍰 ✨
124 files reformatted, 14236 files left unchanged.

> ruff .
Found 158 errors.

# version 2024.1.1 (stable 2024 style, for some reason 4 more lines too long than stable 2023 style)
> black .
All done! ✨ 🍰 ✨
747 files reformatted, 13613 files left unchanged.

> ruff .
Found 162 errors. -> bunch of E501 Line too long (121 > 120)

# 2024 preview mode (for 2025)
> black . --preview
INTERNAL ERROR: Black produced code that is not equivalent to the source.  
Oh no! 💥 💔 💥
1121 files reformatted, 13238 files left unchanged, 1 file failed to reformat.

> black . --preview --check
INTERNAL ERROR: Black produced code that is not equivalent to the source.  
14359 files would be left unchanged, 1 file would fail to reformat.

> ruff .
Found 162 errors. (bunch of E501 Line too long (121 > 120) -> same errors with our without preview
>  black --preview --enable-unstable-feature string_processing .
134 files reformatted, 14225 files left unchanged, 1 file failed to reformat.
> ruff .
Found 1 errors. (one too long comment left)

Findings:

  • 2023 + experiment processing == 0 errors in ruff
  • 2023 vs 2024 stable style ... 158 lines too long for 2023, 162 for 2024.
  • 2023 vs 2024 nedds to update 747 files to latest stable style ...
  • 2024 vs 2024 preview is modifying additional 1121 files (around 8% code base) a lot of changes just for couple of features added late in the process.
  • 2024 preview is crashing on one file

The crash:

cat  /var/folders/l6/nmk965l50gv_90_c4xh5x0_4ctpk94/T/blk_1texc93w.log
--- src
+++ dst
@@ -1737,8 +1737,6 @@
     )  # /TypeIgnore
     TypeIgnore(
     )  # /TypeIgnore
     TypeIgnore(
     )  # /TypeIgnore
-    TypeIgnore(
-    )  # /TypeIgnore
 )  # /Module
\ No newline at end of file

@matejsp
Copy link
Author

matejsp commented Feb 5, 2024

Basically we have 5 options:

  • stay on 2023 with experimental string processing
    0 file changes, taking 40 seconds
  • stay on 2023 without experimental string processing (disable too long line checks)
    124 file changes, taking 1 minute 2 seconds
  • move to 2024 stable and disable line too long checks leave them to black lenient "interpretation"
    747 files changes, taking 3 minutes 4 seconds
  • move to 2024 preview ( and with each back release update all of our code based on introduction or removal of featuers
    1726 file changes, 4 minutes 10 seconds
  • ruff format similar to 2024 but not the same (its own intepretation or wip)
    1373 files changed, taking 10 seconds for very first and 2 seconds for consequtive runs

Timing is on 2019 macbook pro with 32gb.

Findings:

  • black with more and more features is getting noticable slower and slower (from current less than a minute to more than 4 minutes!!!). Before you ask ... yes 2024 and 2023 both using compiled optimization.
  • there are multiple styles flavours each changing a lot of files (hard to gradually switch, introducing lots of git conflict in the process, etc)
  • ruff format is still very young breaking the format all the time and not very good at preserving single quotes that we use
  • ruff format written is rust is brutal fast making it the only usable for precommit hook
  • ruff format replaces a lot of legacy u'xxxx' to 'xxxx' making our code a lot cleaner (idea for black)
  • getting to around 250 conflicting files changes between ruff and black (black ruff black ruff ... )

Since all the options are getting equally painful to maintain for us, I think we will eventually go in ruff direction and their own interpreation of black style. But for now we stay with 2023 black. I will monitor #4208 in the meantime.

@JelleZijlstra
Copy link
Collaborator

Your timings are likely distorted by cache effects and by the fact that Black is a lot slower when it changes files (because it does more checks).

@matejsp
Copy link
Author

matejsp commented Feb 5, 2024

Hm ... oh yes I see ... changing files takes a huge amount of time in black.

2024 preview with zero files changed is much better and does not regress between versions:

time black --preview .
All done! ✨ 🍰 ✨
14360 files left unchanged.
black . --preview  10.76s user 6.48s system 55% cpu 30.843 total
time black . --preview -W 4  
14359 files left unchanged, 1 file failed to reformat.
black . --preview -W 4  10.76s user 6.21s system 62% cpu 27.215 total
time ruff format --no-cache
14522 files left unchanged
ruff format --no-cache  11.09s user 2.66s system 384% cpu 3.572 total

Sorry for false alarm ...
I also tried W 4, W 16 W 160, but it does not really work or have a lot of impact (on macosx at least) ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants