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

Restriction parameter to prevent more than 7 parameters in the next PRs #693

Merged
merged 2 commits into from Sep 16, 2022

Conversation

sultan
Copy link
Contributor

@sultan sultan commented Sep 15, 2022

i have a PR waiting to be pushed/suggested but i need these methods to be refactored first so i dont fall into more than 7 parameters compiler errors + the new design i suggest makes the methods more easy to maintain.

i know this does not add any more features but i hope this can be usefull for the quality and ease of maintenance there after.

the basics are sending a Restriction parameter instead of 4 (lower/upper bound and accept booleans), thus gaining 3 more free params to send for other features (next PR to filter RCs is waiting for this one to be accepted)

Thank you for your time

@jarmoniuk
Copy link
Contributor

jarmoniuk commented Sep 15, 2022

You can also suppress the checkstyle error by using

@SuppressWarnings("checkstyle:ParameterNumber")

@sultan
Copy link
Contributor Author

sultan commented Sep 15, 2022

Thanks, good to know there are such options !
i find it quite harsh for me to bypass the checkstyle and have like 4 to 5 booleans in the final methods of the next PRs.

I find it easier to digest to cut my PR into two (this one for the refactoring, and another for the exclude milestone/previews feature)

would it be ok for the team to merge this PR before the next feature, or do you prefer i merge my work into one PR ?

Comment on lines -240 to -248
boolean includeSnapshots = this.allowSnapshots;
if ( allowingSnapshots )
{
includeSnapshots = true;
}
if ( allowingSnapshots )
{
includeSnapshots = false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Interesting ... looks like never was working

Copy link
Contributor

@jarmoniuk jarmoniuk Sep 16, 2022

Choose a reason for hiding this comment

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

No, not really. It kinda worked, but I really doubt its usefulness. I contemplated removing it too, but I saw it being used.

this.allowSnapshots was a Boolean value, could also be null. Local allowingSnaphots was a primitive boolean, but it was a kind of an override for the value in the Mojo.

So, if allowingSnapshots method argument was set, the code overrote the value from the Mojo object with the method argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two methods with Boolean overrides. One of them was with the boolean primitive and seems not used. I did not felt removing this one. Might consider marking deprecated ?

@slawekjaranowski
Copy link
Member

would it be ok for the team to merge this PR before the next feature, or do you prefer i merge my work into one PR ?

simpler PR easier to review - it is ok for me for splitting

@slawekjaranowski slawekjaranowski merged commit 44b8e87 into mojohaus:master Sep 16, 2022
@slawekjaranowski slawekjaranowski added this to the 2.13.0 milestone Sep 16, 2022
@sultan sultan deleted the Fix7Params branch September 16, 2022 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants