troessner / reek

Code smell detector for Ruby
https://github.com/troessner/reek
MIT License
4.05k stars 280 forks source link

Inline exception configuration #17

Closed kevinrutherford closed 14 years ago

kevinrutherford commented 15 years ago

Would be nice to be able to disable smells for specific bits of code instead of using the config file. for example...

def my_method #:nosmell[utility_function]
  ...
end

[Raised by S. Brent Faulkner]

kevinrutherford commented 15 years ago

The latest version of ruby_parser provides access to comments immediately preceeding methods etc. Which means configuration could be done like this:

# :reek: exclude from UtilityFunction
def my_method
  ...
end

I haven't verified this technique yet, and clearly I also need to work on the syntax. But the idea definitely has promise!

kevinrutherford commented 15 years ago

And of course, this idea is predicated on fixing #4 first.

ashmoran commented 15 years ago

This would mean that you couldn't use MyClass.reek? and inline smell exclusions at the same time though, right? Seems like this feature would obsolete ParseTree compatibility.

kevinrutherford commented 14 years ago

The simplest form I can think of would simply disable a smell detector when it gets mentioned in the comments above a code element (module, class or method). So for example:

# This method is a :reek:long_method and has :reek:duplication in it.
def bad_method(blah, ...)
  #...
end

I would want to be able to name smells using either CamelCase or ruby_names. I would also want to disable every detector in a class by naming that class; for example :reek:LowCohesion would disable both UtilityFunction and FeatureEnvy.

The other main option would be to allow any full Reek YAML configuration in the header comments. However, I don't think that's needed: if you know a method has a smell, there's no point finessing a config so that Reek hops around that smell; may as well just turn off the detector. (However, there may be a good argument for allowing full YAML config in the headers for classes...)

Thoughts?

sbfaulkner commented 14 years ago

Maybe we can get the best of both (and avoiding having to handle comments differently for different contexts), by making ":reek:smell" the shorthand equivalent of ":reek:smell:{enabled:false}"

sbfaulkner commented 14 years ago

I've got this idea working in the "inline_config" branch of my fork.

You can use either CamelCase or under_scored smell names.

Also, you can disable a smell by simply mentioning it in a comment as you suggested, but it will handle fully specified yaml options if they are provided.

Check it out at your leisure. I've submitted a pull request in case you think it fits the bill.

Cheers.

kevinrutherford commented 14 years ago

This is a really neat idea. I'll merge your code today.

sbfaulkner commented 14 years ago

Is all that's left to do on this (for now) to make it possible to disable subclasses using the smell class? (for example :reek:LowCohesion would disable both UtilityFunction and FeatureEnvy)

kevinrutherford commented 14 years ago

Yes. But for completeness, that also ought to be done for config files and the spec matchers...

kevinrutherford commented 14 years ago

Peeled these last remaining points off into #67. Remainder fixed in commit 69cc0b4575e27f9e94924f767e023463b7e9c35d ready for release 1.2.8.