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

Separate Darc and PCS codeflow logic from VmrForwardFlower/VmrBackflower #4351

Open
dkurepa opened this issue Jan 21, 2025 · 1 comment
Open

Comments

@dkurepa
Copy link
Member

dkurepa commented Jan 21, 2025

Context

There are two ways to use the codeflow: through CLI and through PCS. For forward flow, we have a class called VmrForwardFlower that implements most of the flow logic. In the CLI scenario, we directly call into this class and preform what we need to.
In the PCS scenario, we have a class on top of this one PcsVmrForwardFlower that does some things of it's own, and then calls into the VmrForwardFlower. This inconsistency makes it a bit hard to read the code.
The same goes for the backwards flow

Goal

Make VmrForwardFlower and VmrBackwardFlower abstract, and add DarcForwardFlower/DarcBackwardFlower to be a class similar to the currently existing PcsVmrFlower classes

@adamzip
Copy link
Contributor

adamzip commented Feb 21, 2025

There's considerable overhead in terms of debugging difficulty and code readability when introducing new abstract classes. Especially given that VmrForwardFlower and VmrBackFlower are themselves already implementations of the abstract class VmrCodeFlower.
If it's confusing that the PCS enters the code flower from PcsVmrForwardFlower and Darc does so from VmrForwardFlower, then how about getting rid of PcsVmrForwardFlower and having eveyone using VmrForwardFlower? In that case, we can have a couple of overloading methods that call the code flow based on the different arguments that Pcs and Darc are able to provide. There's not really a need for creating extra classes just for creating new entry points into the codeflow.

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

No branches or pull requests

2 participants