From 110ad3898c5a0ceea6f8f57995ec16db7f2edcde Mon Sep 17 00:00:00 2001 From: Krishnan Chandra <1229365+krishnan-chandra@users.noreply.github.com> Date: Mon, 19 Feb 2024 20:15:13 -0500 Subject: [PATCH] Fix update build files formatter selection (#20580) 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. --- .../pants/core/goals/update_build_files.py | 11 ++++++++-- .../core/goals/update_build_files_test.py | 22 ++++++++++++++++++- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/python/pants/core/goals/update_build_files.py b/src/python/pants/core/goals/update_build_files.py index a1725337f49..f4d96117d78 100644 --- a/src/python/pants/core/goals/update_build_files.py +++ b/src/python/pants/core/goals/update_build_files.py @@ -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) diff --git a/src/python/pants/core/goals/update_build_files_test.py b/src/python/pants/core/goals/update_build_files_test.py index 1bd1b694b38..379179bdc80 100644 --- a/src/python/pants/core/goals/update_build_files_test.py +++ b/src/python/pants/core/goals/update_build_files_test.py @@ -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), ) ) @@ -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], )