Regression fix: leave R prefixes capitalization alone #2285
Conversation
|
I think there's code in |
|
Yeah after #2297 I was wondering why there are essentially two functions for getting docstring prefixes. Looking at their usage it seems that But there is at least one other place which transforms prefixes, in string merging and splitting. |
`black.strings.get_string_prefix` used to lowercase the extracted prefix before returning it. This is wrong because 1) it ignores the fact we should leave R prefixes alone because of MagicPython, and 2) there is dedicated prefix casing handling code that fixes issue 1. `.lower` is too naive. This was originally fixed in 20.8b0, but was reintroduced since 21.4b0. I also added proper prefix normalization for docstrings by using the `black.strings.normalize_string_prefix` helper. Some more test strings were added to make sure strings with capitalized prefixes aren't treated differently (actually happened with my original patch, Jelle had to point it out to me).
ichard26
force-pushed
the
fix-string-prefix-regression
branch
from
ca7a34d
to
c2ce5cf
Compare
| @@ -87,7 +87,7 @@ def get_string_prefix(string: str) -> str: | |||
| prefix = "" | |||
| prefix_idx = 0 | |||
| while string[prefix_idx] in STRING_PREFIX_CHARS: | |||
| prefix += string[prefix_idx].lower() | |||
| prefix += string[prefix_idx] | |||
There was a problem hiding this comment.
I wonder if this could not repeatedly append to the string and just calculate the index, and then slice the whole prefix at once 🤔 Not really a comment to the PR but came to my mind anyway.
There was a problem hiding this comment.
Sadly min(s.find("'"), s.find('"')) doesn't quite work because no matches is returned as -1 😄 and it could be way less efficient
There was a problem hiding this comment.
I'm taking this as that no you have no nits or objections for this PR :)
Thanks for the review though! You're doing great adjusting to your new role!
|
Apart from the comment above, the changes seem good to me at least. Leaving all processing to calling code seems appropriate in a function called |
|
|
||
| fstring = ( | ||
| f"f-strings definitely make things more {difficult} than they need to be for" | ||
| " {black}. But boy they sure are handy. The problem is that some lines will need" |
There was a problem hiding this comment.
I like this style of test
black.strings.get_string_prefixused to lowercase the extractedprefix before returning it. This is wrong because 1) it ignores the
fact we should leave R prefixes alone because of MagicPython, and 2)
there is dedicated prefix casing handling code that fixes issue 1.
.loweris too naive.This was originally fixed in 20.8b0, but was reintroduced since 21.4b0.
(Re)fixes GH-1244.