[#2119] Implement Proper Deep Cloning for RepoConfiguration and CliArguments

[georgetayqy]

Fixes #2119

Proposed commit message

The current implementation of `RepoConfiguration` and `CliArguments` do
not permit Builder reuse. When a Builder `build` an instance of
`RepoConfiguration` or `CliArguments`, the stored instance of
`RepoConfiguration` or `CliArguments` within the Builder objects is
dereferenced and reset to the base instance of `RepoConfiguration` or
`CliArguments`.

This might result in unnecessary repetition of code to create duplicate
instances of `RepoConfiguration` or `CliArguments`, if needed.

Let's move to implement a cloning method in both `RepoConfiguration`
and `CliArguments`, to reduce code duplication.

Other information

This issue is currently being discussed, and this PR serves as a proof of concept for implementing clone for RepoConfiguration and CliArguments. Further tests will be done to increase code quality up to par once we have fully discussed the utility of such a cloning function.

[gok99]

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!

[chan-j-d]

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.

[georgetayqy]

@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 clone should we decide to modify RepoConfiguration and any of its dependent classes sometime in the future, complicating maintenance and hampering the extensibility of our code.

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 Cloneable and forcing developers to call a potentially throwing function clone? I believe one of the other initial goal of implementingCloneable was to also minimise side effects and prevent users from accidentally or intentionally altering the contents of a crucial object during runtime. Not sure if this is something that we should look into, but I would love to hear your thoughts on this!

[github-actions]

Hi,
We are going to mark this PR as stale because it has been inactive for the past 30 days.
If no further activity occurs within the following 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days and leave a comment to remove the stale label.
Do let us know if you are stuck so that we can help you!'

[gok99]

perhaps we could look into how we can make some of the crucial classes immutable instead

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 s.DoNotMerge, maybe something like ParkedForLater) for later revisiting.

[georgetayqy]

@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.

[gok99]

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

[github-actions]

Hi,
We are going to mark this PR as stale because it has been inactive for the past 30 days.
If no further activity occurs within the following 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days and leave a comment to remove the stale label.
Do let us know if you are stuck so that we can help you!'