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

Allow deleting rules in plugins to override behavior #21318

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

grihabor
Copy link
Contributor

@grihabor grihabor commented Aug 18, 2024

Sometimes you might want to change the behavior of some existing backend in a way that is not supported by the backend. DeleteRule allows you to remove a single rule from the backend, then you can implement your own version of the same rule.

For example, you might want to change the behavior of the list goal, here is how you do it:

from pants.backend.project_info.list_targets import List, ListSubsystem
from pants.backend.project_info.list_targets import list_targets as original_rule
from pants.engine.addresses import Addresses
from pants.engine.console import Console
from pants.engine.rules import DeleteRule, collect_rules, goal_rule


@goal_rule
async def list_targets(addresses: Addresses, list_subsystem: ListSubsystem, console: Console) -> List:
    with list_subsystem.line_oriented(console) as print_stdout:
        print_stdout("ha cool")
    return List(exit_code=0)


def target_types():
    return []


def rules():
    return (
        *collect_rules(),
        DeleteRule.create(original_rule),
    )

@grihabor grihabor changed the title Allow deleting rules in plugins to override behaviour Allow deleting rules in plugins to override behavior Aug 18, 2024
@huonw
Copy link
Contributor

huonw commented Aug 25, 2024

Thanks for starting this conversation.

From a quick read, I wonder if this won't interact well with our future-state of call-by-name (e.g. #21065). That is, where a backend calls the underlying rule as a function, rather than using the solver to magically choose it.

One way to explore this might be adding an extra layer of rules to the integration test:

@dataclass(frozen=True)
class WrapperUsingCallByNameRequest:
   pass

@rule
def wrapper_using_call_by_name(request: WrapperUsingCallByNameRequest) -> int:
    return await original_rule(IntRequest())

And then call that in the test instead. (If we go forward with this, I think it'd be good to have the integration test use a wrapper like this anyway, to validate that we're swapping out rule calls even within other rules, not just at the top level, since that's the purpose of this feature.)

If that doesn't work there's a chance that using patching, e.g. unittest.mock.patch, will work.

Can you experiment?

@huonw
Copy link
Contributor

huonw commented Sep 11, 2024

We've just branched for 2.23, so merging this pull request now will come out in 2.24, please move the release notes updates to docs/notes/2.24.x.md. Thank you!

@huonw
Copy link
Contributor

huonw commented Sep 19, 2024

(In case it's not clear, please let us know when this is ready for re-review with a comment. 😄 )

@grihabor
Copy link
Contributor Author

Yeah, thanks, it's not working yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants