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

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

Open
troessner opened this issue Sep 9, 2018 · 3 comments
Open

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

troessner opened this issue Sep 9, 2018 · 3 comments
Assignees

Comments

@troessner
Copy link
Owner

troessner commented Sep 9, 2018

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
Copy link
Collaborator

mvz commented Sep 10, 2018

That sounds like an excellent plan.

@troessner
Copy link
Owner Author

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
Copy link
Owner Author

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

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

2 participants