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

Regression fix: leave R prefixes capitalization alone #2285

Merged
merged 2 commits into from Jun 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
Expand Up @@ -11,6 +11,7 @@
- Added `--required-version` option to require a specific version to be running (#2300)
- Fix incorrect custom breakpoint indices when string group contains fake f-strings
(#2311)
- Fix regression where `R` prefixes would be lowercased for docstrings (#2285)

## 21.5b2

Expand Down
5 changes: 3 additions & 2 deletions src/black/linegen.py
Expand Up @@ -226,8 +226,9 @@ def visit_STRING(self, leaf: Leaf) -> Iterator[Line]:
if is_docstring(leaf) and "\\\n" not in leaf.value:
# We're ignoring docstrings with backslash newline escapes because changing
# indentation of those changes the AST representation of the code.
prefix = get_string_prefix(leaf.value)
docstring = leaf.value[len(prefix) :] # Remove the prefix
docstring = normalize_string_prefix(leaf.value, self.remove_u_prefix)
prefix = get_string_prefix(docstring)
docstring = docstring[len(prefix) :] # Remove the prefix
quote_char = docstring[0]
# A natural way to remove the outer quotes is to do:
# docstring = docstring.strip(quote_char)
Expand Down
2 changes: 1 addition & 1 deletion src/black/strings.py
Expand Up @@ -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]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sadly min(s.find("'"), s.find('"')) doesn't quite work because no matches is returned as -1 😄 and it could be way less efficient

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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!

prefix_idx += 1

return prefix
Expand Down
10 changes: 5 additions & 5 deletions src/black/trans.py
Expand Up @@ -411,7 +411,7 @@ def make_naked(string: str, string_prefix: str) -> str:
and is_valid_index(next_str_idx)
and LL[next_str_idx].type == token.STRING
):
prefix = get_string_prefix(LL[next_str_idx].value)
prefix = get_string_prefix(LL[next_str_idx].value).lower()
next_str_idx += 1

# The next loop merges the string group. The final string will be
Expand All @@ -431,7 +431,7 @@ def make_naked(string: str, string_prefix: str) -> str:
num_of_strings += 1

SS = LL[next_str_idx].value
next_prefix = get_string_prefix(SS)
next_prefix = get_string_prefix(SS).lower()

# If this is an f-string group but this substring is not prefixed
# with 'f'...
Expand Down Expand Up @@ -541,7 +541,7 @@ def _validate_msg(line: Line, string_idx: int) -> TResult[None]:
return TErr("StringMerger does NOT merge multiline strings.")

num_of_strings += 1
prefix = get_string_prefix(leaf.value)
prefix = get_string_prefix(leaf.value).lower()
if "r" in prefix:
return TErr("StringMerger does NOT merge raw strings.")

Expand Down Expand Up @@ -1036,7 +1036,7 @@ def do_transform(self, line: Line, string_idx: int) -> Iterator[TResult[Line]]:
is_valid_index = is_valid_index_factory(LL)
insert_str_child = insert_str_child_factory(LL[string_idx])

prefix = get_string_prefix(LL[string_idx].value)
prefix = get_string_prefix(LL[string_idx].value).lower()

# We MAY choose to drop the 'f' prefix from substrings that don't
# contain any f-expressions, but ONLY if the original f-string
Expand Down Expand Up @@ -1290,7 +1290,7 @@ def fexpr_slices() -> Iterator[Tuple[Index, Index]]:

yield from _fexpr_slices

is_fstring = "f" in get_string_prefix(string)
is_fstring = "f" in get_string_prefix(string).lower()

def breaks_fstring_expression(i: Index) -> bool:
"""
Expand Down
42 changes: 42 additions & 0 deletions tests/data/long_strings__regression.py
Expand Up @@ -495,6 +495,26 @@ async def foo(self):
f"9. You now have a key to add to `{prefix}set api youtube api_key`"
)

# It shouldn't matter if the string prefixes are capitalized.
temp_msg = (
F"{F'{humanize_number(pos)}.': <{pound_len+2}} "
F"{balance: <{bal_len + 5}} "
F"<<{author.display_name}>>\n"
)

fstring = (
F"We have to remember to escape {braces}."
" Like {these}."
F" But not {this}."
)

welcome_to_programming = R"hello," R" world!"

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 to have the 'f' whereas others do not. This {line}, for example, needs one."

x = F"This is a long string which contains an f-expr that should not split {{{[i for i in range(5)]}}}."


# output


Expand Down Expand Up @@ -1100,3 +1120,25 @@ async def foo(self):
"8. No application restrictions are needed. Click Create at the bottom."
f"9. You now have a key to add to `{prefix}set api youtube api_key`"
)

# It shouldn't matter if the string prefixes are capitalized.
temp_msg = (
f"{F'{humanize_number(pos)}.': <{pound_len+2}} "
f"{balance: <{bal_len + 5}} "
f"<<{author.display_name}>>\n"
)

fstring = f"We have to remember to escape {braces}. Like {{these}}. But not {this}."

welcome_to_programming = R"hello," R" world!"

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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this style of test

f" to have the 'f' whereas others do not. This {line}, for example, needs one."
)

x = (
"This is a long string which contains an f-expr that should not split"
f" {{{[i for i in range(5)]}}}."
)
21 changes: 21 additions & 0 deletions tests/data/string_prefixes.py
Expand Up @@ -6,6 +6,17 @@
r"hello"
fR"hello"


def docstring_singleline():
R"""2020 was one hell of a year. The good news is that we were able to"""


def docstring_multiline():
R"""
clear out all of the issues opened in that time :p
"""


# output


Expand All @@ -16,3 +27,13 @@
b"hello"
r"hello"
fR"hello"


def docstring_singleline():
R"""2020 was one hell of a year. The good news is that we were able to"""


def docstring_multiline():
R"""
clear out all of the issues opened in that time :p
"""