-
Notifications
You must be signed in to change notification settings - Fork 267
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
Conversation
… in the next PRs.
… in the next PRs.
You can also suppress the checkstyle error by using
|
Thanks, good to know there are such options ! 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 ? |
boolean includeSnapshots = this.allowSnapshots; | ||
if ( allowingSnapshots ) | ||
{ | ||
includeSnapshots = true; | ||
} | ||
if ( allowingSnapshots ) | ||
{ | ||
includeSnapshots = false; | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
simpler PR easier to review - it is ok for me for splitting |
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