[#2119] Implement Proper Deep Cloning for RepoConfiguration and CliArguments
#2124
Conversation
There was a problem hiding this comment.
Thanks for your work on this throrough proof-of-concept!
As it stands, I am still a little ambivalent about this feature, largely because it seems tricky to maintain and at the moment has no clear use cases.
What we're trying to avoid is having to rewrite the same builder on subsequent use (perhaps builders should be unapologetically single use in this sense and error on subsequent builds instead of simply resetting the instance). However, given that this doesn't happen in a way that causes code clutter (only repeated a small number of times or in a loop), this seems much more readable and maintainable than cascading Cloneable.
Possibly null / immutable fields make writing clone not as trivial as equals, and requires a fair bit of testing to check. I am not sure if all this work is ultimately worth it - let me know what you think!
|
Dropping my two cents, I mostly agree with what @gok99 says. I think we can start with some possible uses of such deep cloning features in testing to see if it is worth the merit. If I understand the changes correctly, there is considerable overhead to maintain this functionality, for example if we wish to add or change some of the fields of a Cloneable class, we would have to also update the clone() method and its tests. Any additional new class used in RepoConfiguration would also require it. The main utility is likely in writing tests, as a common template builder can be cloned multiple times to be used in different tests working on a similar RepoConfiguration/CliArguments but with a few fields changed. Correct me if I'm wrong, but the current changes to test code in this PR is testing the cloning method itself. Perhaps, we could see if there are test classes/methods where the cloning can be used in place of what is there. |
|
@gok99 @chan-j-d Apologies for the late reply, but thank you for your feedback and comments! I agree with @gok99 in that this may be rather troublesome to maintain, and perhaps we should make our builders explicitly one-use only. I also agree with @chan-j-d in that it would require constant updates to After reviewing the codebase, I too realise that cascading cloning offers little benefit to the overall efficiency of the codebase; perhaps we could look into how we can make some of the crucial classes immutable instead, choosing to minimise side effects rather than explicitly implementing |
|
Hi, |
I think having more immutability across the board would be great, but would require some significant architectural changes that should probably happen in smaller stages. @reposense/active-reviewers-1 Regarding the work in this PR (and other PRs like #1094 and #1454 which have been idle for up to 4 years) - PRs of this kind seem to be accumulating in the open PR section. Perhaps we can close them and give them a special tag (more descriptive than |
|
@gok99 Perhaps we could first take a look at how we can make our model classes immutable before we take a look at the other classes. I believe the model classes would be the easiest (and with minimal impacts on the entire codebase) classes to refactor first. |
Yes, we should start with the model classes but that doesn't seem entirely simple to me either. Perhaps you can create an issue with a checklist to track the immutability refactors for the model classes |
Fixes #2119
Proposed commit message
Other information
This issue is currently being discussed, and this PR serves as a proof of concept for implementing
cloneforRepoConfigurationandCliArguments. Further tests will be done to increase code quality up to par once we have fully discussed the utility of such a cloning function.