Fix update build files formatter selection

[krishnan-chandra]

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:

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:

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.

[huonw]

Thank you!

[huonw]

Oh, this is subtle, and I think it was due to my suggestion from before. Thank you for debugging it, and sorry for the incorrect idea.

What do you think about flipping the ifs so we can get an if/else chain based on the request class and so they become "obviously" mutually exclusive?

if request == chosen_formatter_request_class:
    if update_build_files_subsystem.fmt:
        rewrite_request_classes.append(request)
else if issubclass(request, DeprecationFixerRequest):
    if update_build_files_subsystem.fix_safe_deprecations:
        rewrite_request_classes.append(request)
else:
    rewrite_request_classes.append(request)

I guess the next step in DRYing it might be something like:

if request == chosen_formatter_request_class:
    request_enabled = update_build_files_subsystem.fmt
else if issubclass(request, DeprecationFixerRequest):
    request_enabled = update_build_files_subsystem.fix_safe_deprecations
else:
    request_enabled = True

if request_enabled:
    rewrite_request_classes.append(request)

But I dno if that's any better (and, as we've learned, don't trust my ideas without double checking them!).

[krishnan-chandra]

I think both suggestions have a bug in them:

if request == chosen_formatter_request_class:
    if update_build_files_subsystem.fmt:
        rewrite_request_classes.append(request)
else if issubclass(request, DeprecationFixerRequest):
    if update_build_files_subsystem.fix_safe_deprecations:
        rewrite_request_classes.append(request)
else:
    rewrite_request_classes.append(request)

If Black is the chosen formatter and FormatWithRuffRequest is the request being checked here, it would fall through to the final else clause and get added incorrectly.

[krishnan-chandra]

I can't say that the version of the logic added in this PR is the best version, but it's the one that made the most sense in my head and seemed simpler to debug later if needed 😄

[huonw]

If Black is the chosen formatter and FormatWithRuffRequest is the request being checked here, it would fall through to the final else clause and get added incorrectly.

Oh! You're right. Too hard for me, it seems 🤯

[WorkerPants]

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

✔️ 2.20.x

Successfully opened #20582.

---

Thanks again for your contributions!

🤖 Beep Boop here's my run link