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

[CT-2459] Move ParsedNode.patch and Macro.patch to respective PatchParsers #7430

Closed
Tracked by #7372
MichelleArk opened this issue Apr 21, 2023 · 0 comments · Fixed by #7640
Closed
Tracked by #7372

[CT-2459] Move ParsedNode.patch and Macro.patch to respective PatchParsers #7430

MichelleArk opened this issue Apr 21, 2023 · 0 comments · Fixed by #7640
Assignees
Labels
tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality

Comments

@MichelleArk
Copy link
Contributor

Currently a couple node types (Macro, ParsedNode) implement their own patch method which accepts a patch (either ParsedNodePatch or ParsedMacroPatch) and are responsible for applying the contents of the patch to the node attributes. The PatchParsers then call these node patch methods once after constructing a patch.

This splits the overall patching responsibilities into creating a patch and applying it across the PatchParsers and Nodes (which is not really that clean of a split in reality because NodePatchParser implement its own patch_node_config method that applies patches to node configs specifically). Most of the complexity is in creating a patch rather than 'applying' it, and this split requires changes to patching functionality in multiple places each time a new patch attribute is introduced. Additionally, this makes the PatchParsers difficult to test because applying actual result of their processing is mocked out.

Let's move the Node.patch methods into the respective PatchParsers instead to consolidate overall patching logic. We should also consider creating a separate ParsedModelPatch for model-specific configs that are patched by ModelPatchParser if possible (could be tricky with the inheritance currently in place).

@MichelleArk MichelleArk added tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality Team:Language labels Apr 21, 2023
@github-actions github-actions bot changed the title Move ParsedNode.patch and Macro.patch to respective PatchParsers [CT-2459] Move ParsedNode.patch and Macro.patch to respective PatchParsers Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality
Projects
None yet
2 participants