plafer
commented
2 days ago More generally, I'd like to understand what is the overall plan for the refactoring.
I saw this PR as a cleanup that we'd want no matter ultimate direction of the subsequent PR. The main contribution is to remove the AdviceProvider::set_advice() and AdviceProvider::get_advice(). For example, set_advice() shoehorned all possible advice injector functionality into a single method call, and then aggregated all possible return types into this HostResponse (with From implementations that would panic if the wrong type is returned). Basically I changed it so that the caller calls the appropriate AdviceProvider method instead, and get the appropriate return type directly instead of through this HostResponse, removing all that complexity. Similarly for AdviceProvider::get_advice().
Then the Host re-exposed those set_advice() and get_advice() methods, blurring the line between Host and AdviceProvider. In the current approach, the Host exposes the advice provider, and the caller can the interact with it directly. It makes for a cleaner separation where the Host manages the advice provider, rather than "being one". It's also easier to refactor - if we change the AdviceProvider, we don't have to duplicate the same changes on the Host.
Finally I inlined all the AdviceProvider default methods to remove a level of indirection - most of these were just a few lines.
Depending on how we go, it seems to me that a subsequent PR may need to reverse many of the changes in this PR.
For example, I'm imagining that Host::on_event() method will handle the actual dispatch of advice injectors. It could look something like:
That's fine, I don't think this undoes any of the changes here - just more moving things around.
If we go this way, instead of removing individual advice injector functions, we'd remove the corresponding functions from the AdviceProvider trait.
I agree with this direction - I'd like the AdviceProvider trait to look as closely as our high-level description of it as possible. I thought about making the AdviceProvider trait have only 3 methods: return a stack, map, and store. But then this would be a lot of changes and this PR was already decently big.
Builds on #1571
A round of refactoring to make the PR that addresses #1457 more self contained.