I think this is useful too.
It allows us to do rescue e => and map { |x| x.do_something }

  UncommunicativeVariableName:
    accept:
     - e
     - x

sounds good

I wonder if we should also add _ for those times we have a unused variable?

added these 3 accepted short names

I wonder if we should add k and v as they are common short names for key and value when iterating over a hash

Sounds good to me

would it be possible to propose a new rule? if so, where is the best place to do it?

comments here in the PR is the place for that

Is this enough for tests?

what do you mean?

Maybe I am wrong, but from what I remember the duplicate method calls happen often in tests when you are calling a method several times??

I just tried calling a method multiple times in a single test and reek doesn't complain about it

I'm not sure how it works internally but it seems it won't complain during tests? we can always change this later to disable it for the test and spec directories I guess if it starts creating issues (maybe there's some edge case I don't know how to trigger)

I'm curious about this part, while I understand not wanting to slow down development in these smaller, newer projects...at what point do you add this, and how much time does it then take to go back and make sure everything is agreeing with reek standards? Where will the line fall for us?

I feel like this is answered below, somewhat, but now I'm wondering at what point do we consider a project is not an MVP/Prototype or a one off internal tool that might be obsolete in a year...

cc @etagwerker @ABizzinotto here, I don't know if we have a clear definition of when this crosses that line (for example, in that list we are not listing UpgradeJS.com)

one thing though is that when we want to add reek to a project, it can be added with TODOs to address things gradually, it doesn't need to be something like "we add reek and now we have to fix all the smells", it's more like "we add reek, and we can address the smells gradually over time along with other changes"

Won't that break the build though?

if we add the reek ignore comments it shouldn't break the builds, it would be useful to flag the comments as something that is agreed to be ignore vs the ones we ignore to deal with them later with a TODO

I'm wondering if it wouldn't be better to have it be true. Even in controllers and models, if it's unused, wouldn't we want to remove them?

the problem with this rule is that (at least from my tests), it's not smart enough to detect that you are referencing a private method in a callback, as in before_action :some_private_method on a controller or an after_save :do_something_private in a model

we have that enabled above in general, but we are disabling them for controllers and models

it was showing many false positives saying a private method was not used when it was used in a callback

I think I'd like some examples of this. I don't know of any off the top of my head, but I'm only thinking that, situationally, if a smell requires a fix that we know the client doesn't really care about or that, in our evaluation, isn't worth the time out of the project it would take to fix it, then we could skip it.

Is that it?

this is not for client project, it's only for our internal projects

and example here will be that with these defaults, when we run reek in fastruby.io/ombulabs.com/points we will have reek code smells, if something reek complains about is not worth the time to fix it (maybe a method is long because it has to be long, or maybe it makes total sense to have a utility function in a file and moving it somewhere else would make it less readable), then we can add reek comments to skip it

another example could be a private method in some class that is not used explicitly but because it's used by some send(...) call with variables, in that cases reek wouldn't know it's actually used

the main idea for this is to not encourage people to just add :reek: comments because they didn't feel like fixing a reek warning, if something won't be addressed there has to be a good argument for that (good as in the people working on that task/pr/story agree with)

and this can also help use improve these defaults, if we see something that gets skipped a lot, we might be able to change these defaults based on those reasons, but we need a reason

I wonder if we should also add _ for those times we have a unused variable?

In the past I have found the following helpful in controllers too:

    FeatureEnvy:
      enabled: false
    TooManyMethods:
      enabled: false
    TooManyStatements:
      enabled: false

FeatureEnvy: It is easy for controller methods to interact with other parts of the application
TooManyMethods: Controllers can easily go over 15 methods
TooManyStatements: Controllers methods easily go over 5 lines

do you think those are better fully disabled or maybe add as exceptions when needed?

I'm running reek on fastruby with the current state of this proposed config and I don't get any TooManyMethods nor FeatureEnvy warnings for controllers, and only 2 TooManyStatements in one of the controllers (maybe those actions can be ignored as exceptions or refactored)

maybe we can disable TooManyMethods for controllers and models too? I don't think it adds that much value (I think we are already good at not putting methods where they don't belong, and when they are in the right place it would be annoying to have reek complain just because of the number)

I am onboard with disabling TooManyMethods in controllers/models yes.
And perhaps you are right, we can disable feature envy and too many statements where needed.

- db/migrate In the past I ignored the db/migrate directory too

we are already disabling many checks for the db/migrate directory above, do you think it's better to just ignore everything instead?

I don't know if there is much value in reeking db/migrate file at all.
They typically fall outside of the normal application - and should not really contain business logic at all.
So I would say yes, let's disable all of the rules in this directory.