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.

Sign up for GitHub

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

Jump to bottom

[#2117] Refactor CliArguments to conform to RepoConfiguration's Builder Pattern #2118

Merged
merged 3 commits into from
Mar 2, 2024
Merged

[#2117] Refactor CliArguments to conform to RepoConfiguration's Builder Pattern #2118

merged 3 commits into from
Mar 2, 2024

Conversation

asdfghjkxd
Copy link
Contributor

@asdfghjkxd asdfghjkxd commented Feb 17, 2024 •
edited
Loading

Fixes #2117

Proposed commit message

Current implementation of the Builder pattern within `CliArguments`
does not conform that in `RepoConfiguration`.

Let's move to refactor `CliArguments` to reduce class attribute
duplication across `CliArguments` and `CliArguments::Builder` and to
enhance overall code quality.

Other information

N/A

@asdfghjkxd asdfghjkxd requested a review from a team February 17, 2024 10:42
sopa301
sopa301 approved these changes Feb 18, 2024
View reviewed changes
Copy link
Contributor

@sopa301 sopa301 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though I have a minor query.

src/main/java/reposense/model/CliArguments.java
@@ -508,7 +461,9 @@ public Builder reportConfiguration(ReportConfiguration reportConfiguration) {
* @return CliArguments
*/
public CliArguments build() {
return new CliArguments(this);
CliArguments built = this.cliArguments;
this.cliArguments = new CliArguments();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I clarify why we choose to reset cliArguments here?

Copy link
Contributor Author

@asdfghjkxd asdfghjkxd Feb 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! The main motivation behind resetting the Builder object on every build operation is to prevent aliasing issues when the user chooses to rebuild CliArguments multiple times. Since the cloning operation has not been implemented yet, we decided that the temporary fix would be to reset the Builder object on every build.

A new issue has been released which aims to tackle this issue and to allow duplication of CliArguments to avoid resetting the Builder object on every build. I am working on it and will release the PR sometime this coming week!

@asdfghjkxd asdfghjkxd requested a review from a team February 19, 2024 04:07
gok99
gok99 approved these changes Feb 19, 2024
View reviewed changes
Copy link
Contributor

@gok99 gok99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, could you also link this PR in issue #2119

@asdfghjkxd asdfghjkxd mentioned this pull request Feb 19, 2024
Open
@asdfghjkxd
Copy link
Contributor Author

asdfghjkxd commented Feb 19, 2024 •
edited
Loading

Sure thing, I have linked this issue within issue #2119!

@ckcherry23 ckcherry23 requested a review from a team February 19, 2024 06:31
MarcusTXK
MarcusTXK approved these changes Feb 23, 2024
View reviewed changes
Copy link
Member

@MarcusTXK MarcusTXK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for standardising the builders

@ckcherry23 ckcherry23 merged commit bbb2f69 into reposense:master Mar 2, 2024
10 checks passed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 2, 2024

The following links are for previewing this pull request:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gok99 gok99 gok99 approved these changes

@MarcusTXK MarcusTXK MarcusTXK approved these changes

@sopa301 sopa301 sopa301 approved these changes

Assignees
No one assigned
Labels
s.ToMerge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor CliArguments to conform to RepoConfiguration's Builder Pattern
5 participants
@asdfghjkxd @gok99 @MarcusTXK @sopa301 @ckcherry23