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

Disable the UnusedPrivateMethod smell by default #844

Closed
troessner opened this issue Jan 30, 2016 · 5 comments
Closed

Disable the UnusedPrivateMethod smell by default #844

troessner opened this issue Jan 30, 2016 · 5 comments

Comments

@troessner
Copy link
Owner

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 ?

@mvz
Copy link
Collaborator

mvz commented Jan 30, 2016

These two bug reports are very different, so it may be easier to discuss them separately and only then come back to this question.

@mvz
Copy link
Collaborator

mvz commented Jan 31, 2016

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.

@jdickey
Copy link

jdickey commented Feb 1, 2016

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.

@mvz
Copy link
Collaborator

mvz commented Feb 1, 2016

beating your head against whitequark/parser

I'm afraid the fault for #845 lies entirely with us.

@troessner
Copy link
Owner Author

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.

Agreed, I'll whip up a PR soon.

and you're nowhere near as smart as all the interpreters you're testing against.

It doesn't have to be that smart ;)
E.g. dynamic dispatch is something we'll probably never figure out and do not intend to as well.
But the problems listed above is certainly something that should be possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants