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

Exclusions while generating the TODO file #1406

Closed
dmarcoux opened this issue Aug 21, 2018 · 9 comments
Closed

Exclusions while generating the TODO file #1406

dmarcoux opened this issue Aug 21, 2018 · 9 comments
Assignees
Labels

Comments

@dmarcoux
Copy link

Hey,

I would like to start using reek in a project. I want reek to skip some directories/paths as they contain legacy files.

I cannot find a way to generate the TODO file while excluding those directories/paths. Is this possible?

@troessner
Copy link
Owner

Yes, you can just use the regular "exclude_path" option in your configuration file that is outlined here, so e.g.:

# Directories below will not be scanned at all
exclude_paths:
  - lib/legacy
  - lib/rake/legacy_tasks

Reek will use the same configuration it uses for regular runs for the generation of the todo list as well.

@dmarcoux
Copy link
Author

dmarcoux commented Aug 21, 2018

This part of the README confuses me:

This means that when you run

`reek -c other_configuration.reek --todo lib/`

`other_configuration.reek` will simply be ignored (as outlined before, Reek is supposed to have one configuration file and one file only).

So this is what I do to generate the .todo.reek file. I first create my reek config (with the default name .reek.yml) with the following content:

exclude_paths:
  - lib/legacy
  - lib/rake/legacy_tasks

After this, I run reek --todo. .todo.reek is now generated. I am supposed to run reek with one configuration file only, but I have now two: .reek.yml and .todo.reek. Which one am I supposed to use? .reek.yml contains only the exclude_paths part while .todo.reek contains only the exclusions for the various detectors (so the TODOs). Running reek will trigger for all the warnings already listed in .todo.reek and running reek -c .todo.reek will trigger warnings for all the files under the exclude_paths contained in .reek.yml.

@troessner
Copy link
Owner

troessner commented Aug 22, 2018

Yeah, you're right, that IS super confusing and we really should fix this 😆

So the way that this could work is that you copy the whole content of .todo.reek into .reek.yml. But that is very inconvenient for sure, no question about that.

@mvz wdyt having the TodoCommand recognise 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?

Coupled with explanatory output like

No configuration file found or given, so we're creating a new .todo.reek for you

or

You are using the configuration file ..... so we're going to append to that

we might be able to remove all confusion.

@mvz
Copy link
Collaborator

mvz commented Aug 22, 2018

Yeah that's probably the best solution.

@dmarcoux
Copy link
Author

Is there a reason behind having only one configuration file?

I find the approach of Rubocop with inherit_from quite convenient and straightforward. The separation is clear between the configuration and the TODOs.

@troessner
Copy link
Owner

Is there a reason behind having only one configuration file?

Yes, we discussed this quite extensively and the answer is that we see more benefit in a very simple and straight forward configuration handling (which includes our own source code complexity) than in supporting every use case of multiple configuration files. I do agree though that your use case is a very good one which we'll hopefully fix with the solution outlined above. I'll probably look into implementing this this weekend.

@troessner
Copy link
Owner

troessner commented Oct 7, 2018

Closing this one since the initial question has been answered.
Regarding your problems with the --todo flag, we recently fixed this in the latest version. It now works like this:

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.

We will make it possible to include additional configuration files in the future. See #1411 for details ;)

@mvz
Copy link
Collaborator

mvz commented Oct 8, 2018

@troessner maybe triple-backticks are not ideal for quoting human text 😉.

@troessner
Copy link
Owner

@mvz good point, fixed, thanks ;)

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

No branches or pull requests

3 participants