troessner / reek

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

A strategy for making our --todo feature more useful #1411

Open troessner opened 6 years ago

troessner commented 6 years ago

As I mentioned in #1406 I have been working on improving our todo list functionality so that the TodoCommand recognises if there is a configuration file present (either the default one or whatever is passed in via -c option) and if there is, append to that instead of creating a new file.

Unfortunately I hit a snag recently: I thought I could just add another detectors block to our configuration file like this:

      ---
      detectors:
        UncommunicativeMethodName:
          exclude:
          - Smelly#x
      # Auto generated by Reeks --todo flag
      detectors:
        UncommunicativeVariableName:
          exclude:
          - Smelly#x

but this doesn't work because ConfigurationFileFinder.load_from_file just discards all non-unique keys.

I see 3 options to fix this:

1.) Find the current detectors block and append the todo list configuration there. This might become very messy very fast, since we'd have take into account all different configuration file layouts there could be. Looking for markers like "last 4 characters indented line in the first detectors block" sounds already incredibly hacky and error-prone.

2.) Read in the existing configuration, append the new todo list configuration and then write it out again. This might kill the formatting of the original file and delete comments. Both not acceptable obviously.

3.) Add the feature to Reek that allows to include additional configuration file via include directive.

What I'd like to suggest:

1.) Update the todo list generation so that it will only update the .reek.yml based on no existing configuration (which corresponds to what we have already since the current TodoList command does not take into account any pre-existing configuration). The task will just abort with a corresponding message if there already is a .reek.yml present. So super simple and just suited for the use case where you introduce Reek into a legacy project. This will also make our todo list feature extremely simple to understand and explain and remove the confusion for now.

2.) Add the feature to Reek that allows to include additional configuration files via include directive and reverse-merge whatever it finds in those files.

3.) Come back to the initial idea of improving the todo list generation and basically continue to work on what I start in the PR #1408 which would make the todo list feature very powerful since you could not just use it for introducing it to legacy projects but also to continuously work on bigger projects where you might not have control over people fixing warnings and you need to get back to a clean slate to continue.

WDYT?

mvz commented 6 years ago

That sounds like an excellent plan.

troessner commented 6 years ago

Glad you like it, I'll work on this the next couple of weeks. [1] is trivial and [3] is easy given [2]. So [2] is gonna be the big one ;)

troessner commented 6 years ago

I'll tackle [2] in the next couple of days / weeks ;)