Skip to content

Commit 73d0187

Browse files
Fix update build files formatter selection (Cherry-pick of #20580) (#20582)
Closes #20576. Although this is a relatively small change, I think it's good to understand the change in context so I've included the new and old logic below for comparison. New logic: ```py formatter_to_request_class: dict[Formatter, type[RewrittenBuildFileRequest]] = { Formatter.BLACK: FormatWithBlackRequest, Formatter.YAPF: FormatWithYapfRequest, Formatter.RUFF: FormatWithRuffRequest, } chosen_formatter_request_class = formatter_to_request_class.get( update_build_files_subsystem.formatter ) if not chosen_formatter_request_class: raise ValueError(f"Unrecognized formatter: {update_build_files_subsystem.formatter}") for request in union_membership[RewrittenBuildFileRequest]: if update_build_files_subsystem.fmt and request == chosen_formatter_request_class: rewrite_request_classes.append(request) if update_build_files_subsystem.fix_safe_deprecations and issubclass( request, DeprecationFixerRequest ): rewrite_request_classes.append(request) # If there are other types of requests that aren't the standard formatter # backends or deprecation fixers, add them here. if request not in formatter_to_request_class.values() and not issubclass( request, DeprecationFixerRequest ): rewrite_request_classes.append(request) ``` Old logic: ```py for request in union_membership[RewrittenBuildFileRequest]: if issubclass(request, (FormatWithBlackRequest, FormatWithYapfRequest)): is_chosen_formatter = issubclass(request, FormatWithBlackRequest) ^ ( update_build_files_subsystem.formatter == Formatter.YAPF ) if update_build_files_subsystem.fmt and is_chosen_formatter: rewrite_request_classes.append(request) else: continue if update_build_files_subsystem.fix_safe_deprecations or not issubclass( request, DeprecationFixerRequest ): rewrite_request_classes.append(request) ``` The `else: continue` in the old logic was load-bearing, because it would skip over the second top-level `if` statement and move to the next loop iteration in cases where the request class was one of the regular formatters (`FormatWithBlackRequest` or `FormatWithYapfRequest`). The `or not issubclass(request, DeprecationFixerRequest)` was intended to catch formatters that live outside of the list of formatter backends and deprecation fixers - the last `if` statement in the new logic is intended to cover this case as well. Co-authored-by: Krishnan Chandra <[email protected]>
1 parent d55b718 commit 73d0187

File tree

2 files changed

+30
-3
lines changed

2 files changed

+30
-3
lines changed

src/python/pants/core/goals/update_build_files.py

+9-2
Original file line numberDiff line numberDiff line change
@@ -234,10 +234,17 @@ async def update_build_files(
234234
raise ValueError(f"Unrecognized formatter: {update_build_files_subsystem.formatter}")
235235

236236
for request in union_membership[RewrittenBuildFileRequest]:
237-
if update_build_files_subsystem.fmt and issubclass(request, chosen_formatter_request_class):
237+
if update_build_files_subsystem.fmt and request == chosen_formatter_request_class:
238238
rewrite_request_classes.append(request)
239239

240-
if update_build_files_subsystem.fix_safe_deprecations or not issubclass(
240+
if update_build_files_subsystem.fix_safe_deprecations and issubclass(
241+
request, DeprecationFixerRequest
242+
):
243+
rewrite_request_classes.append(request)
244+
245+
# If there are other types of requests that aren't the standard formatter
246+
# backends or deprecation fixers, add them here.
247+
if request not in formatter_to_request_class.values() and not issubclass(
241248
request, DeprecationFixerRequest
242249
):
243250
rewrite_request_classes.append(request)

src/python/pants/core/goals/update_build_files_test.py

+21-1
Original file line numberDiff line numberDiff line change
@@ -74,12 +74,24 @@ def reverse_lines(request: MockRewriteReverseLines) -> RewrittenBuildFile:
7474
def generic_goal_rule_runner() -> RuleRunner:
7575
return RuleRunner(
7676
rules=(
77-
update_build_files,
7877
add_line,
7978
reverse_lines,
79+
format_build_file_with_ruff,
80+
format_build_file_with_yapf,
81+
update_build_files,
82+
*config_files.rules(),
83+
*pex.rules(),
84+
# Ruff and Yapf are included, but Black isn't because
85+
# that's the formatter we enable in pants.toml.
86+
# These tests check that Ruff and Yapf are NOT invoked,
87+
# but the other rewrite targets are invoked.
88+
*Ruff.rules(),
89+
*Yapf.rules(),
8090
*UpdateBuildFilesSubsystem.rules(),
8191
UnionRule(RewrittenBuildFileRequest, MockRewriteAddLine),
8292
UnionRule(RewrittenBuildFileRequest, MockRewriteReverseLines),
93+
UnionRule(RewrittenBuildFileRequest, FormatWithRuffRequest),
94+
UnionRule(RewrittenBuildFileRequest, FormatWithYapfRequest),
8395
)
8496
)
8597

@@ -204,12 +216,20 @@ def black_rule_runner() -> RuleRunner:
204216
return RuleRunner(
205217
rules=(
206218
format_build_file_with_black,
219+
format_build_file_with_ruff,
220+
format_build_file_with_yapf,
207221
update_build_files,
208222
*config_files.rules(),
209223
*pex.rules(),
210224
*Black.rules(),
225+
# Even though Ruff and Yapf are included here,
226+
# only Black should be used for formatting.
227+
*Ruff.rules(),
228+
*Yapf.rules(),
211229
*UpdateBuildFilesSubsystem.rules(),
212230
UnionRule(RewrittenBuildFileRequest, FormatWithBlackRequest),
231+
UnionRule(RewrittenBuildFileRequest, FormatWithRuffRequest),
232+
UnionRule(RewrittenBuildFileRequest, FormatWithYapfRequest),
213233
),
214234
target_types=[GenericTarget],
215235
)

0 commit comments

Comments
 (0)