Skip to content

Commit

Permalink
Fix update build files formatter selection (#20580)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
krishnan-chandra authored Feb 20, 2024
1 parent 5779896 commit 110ad38
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 3 deletions.
11 changes: 9 additions & 2 deletions src/python/pants/core/goals/update_build_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -234,10 +234,17 @@ async def update_build_files(
raise ValueError(f"Unrecognized formatter: {update_build_files_subsystem.formatter}")

for request in union_membership[RewrittenBuildFileRequest]:
if update_build_files_subsystem.fmt and issubclass(request, chosen_formatter_request_class):
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 or not issubclass(
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)
Expand Down
22 changes: 21 additions & 1 deletion src/python/pants/core/goals/update_build_files_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,24 @@ def reverse_lines(request: MockRewriteReverseLines) -> RewrittenBuildFile:
def generic_goal_rule_runner() -> RuleRunner:
return RuleRunner(
rules=(
update_build_files,
add_line,
reverse_lines,
format_build_file_with_ruff,
format_build_file_with_yapf,
update_build_files,
*config_files.rules(),
*pex.rules(),
# Ruff and Yapf are included, but Black isn't because
# that's the formatter we enable in pants.toml.
# These tests check that Ruff and Yapf are NOT invoked,
# but the other rewrite targets are invoked.
*Ruff.rules(),
*Yapf.rules(),
*UpdateBuildFilesSubsystem.rules(),
UnionRule(RewrittenBuildFileRequest, MockRewriteAddLine),
UnionRule(RewrittenBuildFileRequest, MockRewriteReverseLines),
UnionRule(RewrittenBuildFileRequest, FormatWithRuffRequest),
UnionRule(RewrittenBuildFileRequest, FormatWithYapfRequest),
)
)

Expand Down Expand Up @@ -204,12 +216,20 @@ def black_rule_runner() -> RuleRunner:
return RuleRunner(
rules=(
format_build_file_with_black,
format_build_file_with_ruff,
format_build_file_with_yapf,
update_build_files,
*config_files.rules(),
*pex.rules(),
*Black.rules(),
# Even though Ruff and Yapf are included here,
# only Black should be used for formatting.
*Ruff.rules(),
*Yapf.rules(),
*UpdateBuildFilesSubsystem.rules(),
UnionRule(RewrittenBuildFileRequest, FormatWithBlackRequest),
UnionRule(RewrittenBuildFileRequest, FormatWithRuffRequest),
UnionRule(RewrittenBuildFileRequest, FormatWithYapfRequest),
),
target_types=[GenericTarget],
)
Expand Down

0 comments on commit 110ad38

Please sign in to comment.