-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[java] UseObjectForClearerAPI Only For Public #1752
[java] UseObjectForClearerAPI Only For Public #1752
Conversation
Generated by 🚫 Danger |
@Vampire thanks for the PR! You are absolutely right about this. Would you care to include a unit test to make sure the issue is never reintroduced in the future? Just adding a new Thanks in advance! |
Sure, I'll have a look. One question though, is it intended that |
And if not, is it an error of the rule that is uses |
@Vampire The ReferenceType node has an attribute Quite honestly I don't see the point of this method and why it only counts strings. There's already a rule TooManyParameters or ParameterCount idk whose proposed correction is to use an object. Maybe that one should be enriched with an option to ignore private methods instead, and this rule dropped. Is there something I'm not seeing about why string parameters in particular are bad? |
I don't know, I just wondered about why it complains about a private method when the description said it only checks public methods. So independent of whether this gets deprecated and replaced by some other rule, when I'm at fixing this, should I also check the array depth as the description says it is only about |
f710edb
to
f3c0d39
Compare
I pushed a version now that has tests and also checks for array. |
Let me chime in:
|
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.
Looks good, thanks!
The description and implementation of the rule suggest that is should only apply
to public methods, but it is effective against all methods as for the existence
of the
@Public
attribute is tested instead of the value, so the condition isalways true.
With this fix, only public methods are considered like intended.
Fixes #1760