-
Notifications
You must be signed in to change notification settings - Fork 280
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
Disable the UnusedPrivateMethod smell by default #844
Comments
These two bug reports are very different, so it may be easier to discuss them separately and only then come back to this question. |
On second thought, disabling this now is probably a good idea because it will allow us to iron out bugs and get a better idea of the implications of this smell in relative quiet. |
Author of #842 here, just throwing in my two cents; I'd like to follow the discussion on this issue. I agree with @mvz that the two reports are very dissimilar; mine talks about a general-Ruby class-definition issue (this is in a Gem, where Rails is nowhere to be found), and #843 is Rails-specific (and probably not surprising given #842). Global registries suck with a picobar-or-less vacuum; i.e., very hard. I've implemented such things in other, internal, tools before and you're never as smart as whichever interpreter you're using, and you're nowhere near as smart as all the interpreters you're testing against. Defaulting the option to 'disabled' while beating your head against whitequark/parser (and whitequark/ast), which I gather you've already done, seems the sensible thing. Thanks for getting on top of this so quickly. |
I'm afraid the fault for #845 lies entirely with us. |
Agreed, I'll whip up a PR soon.
It doesn't have to be that smart ;) |
Let disable the UnusedPrivateMethod smell by default. There are just too many case where this detector will give a false positive, e.g. #843 and #842 and there is no way we can fix it unless we rewrite the entire backbone of
Reek
to use a global class / method / whatever registry that would allow us to detect those cases.WDYT @chastell @mvz ?
The text was updated successfully, but these errors were encountered: