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

Implement Proper Deep Cloning for RepoConfiguration and CliArguments #2119

Open
georgetayqy opened this issue Feb 17, 2024 · 2 comments · May be fixed by #2124
Open

Implement Proper Deep Cloning for RepoConfiguration and CliArguments #2119

georgetayqy opened this issue Feb 17, 2024 · 2 comments · May be fixed by #2124

Comments

@georgetayqy
Copy link
Contributor

georgetayqy commented Feb 17, 2024

What feature(s) would you like to see in RepoSense

Currently, both Builder patterns implemented in RepoConfiguration and CliArguments are implemented in such a way that the Builder objects are one-use and cannot be reused to build new instances of RepoConfiguration and CliArguments.

This is not ideal since this prevents us from reusing Builder instances to construct duplicate instances of RepoConfiguration or CliArguments, which violates the Builder pattern and incurs an overhead if we would like to construct duplicate copies of RepoConfiguration or CliArguments.

Is the feature request related to a problem?

This issue is related to issues #2076, #2117 and PR #2078, #2118.

If possible, describe the solution

We could change the codebase in such a way that classes used in RepoConfiguration and CliArguments, as well as RepoConfiguration and CliArguments, implement the Cloneable interface and provide an implementation of the clone() method to allow for easier duplication and cloning of RepoConfiguration and CliArguments instances.

If applicable, describe alternatives you've considered

N/A

Additional context

N/A

@gok99
Copy link
Contributor

gok99 commented Feb 19, 2024

It might be worth discussing the possible use-cases for Builder re-use, since this would be a new feature that wasn't previously possible. A clone would have to manually build an object of same semantics so every object down the line would have to do this to allow for deep copies, which would also add significant boilerplate throughout the backend. We might discuss if this tradeoff is worth the code-quality gains.

@georgetayqy
Copy link
Contributor Author

georgetayqy commented Feb 19, 2024

@gok99 I have provided a proof of concept for the clone feature in the draft PR linked to this issue, for your reference.

I agree that allowing deep cloning would introduce a significant amount of boilerplate code (as seen in the draft PR, with the different classes implementing the Cloneable::clone() method to allow cloning of RepoConfiguration and CliArguments. It might also incur additional performance/memory overhead through the creation of multiple referenced objects on the heap.

I'm not too sure where Builders can be reused efficiently at the moment; the only place I am certain would help would be the test cases (test cases can reuse base Builder objects to create more complex objects, reducing the amount of code needed to execute the test cases), but implementing such a drastic feature just for a slight improvement in memory use for unit testing might not be worth the increase in bloat and complexity of the codebase coupled with the increase in code quality...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants