DT-462: Add RFC for Reek configuration #5

arielj · 2025-06-02T19:48:11Z

Read the .md file included in the DIFF for details.

You can see the parsed version of the file at https://github.com/fastruby/RFCs/blob/DT-462-shared-default-reek-configuration/2025-06-02-default-reek-configuration.md

fbuys · 2025-08-28T14:51:00Z

2025-06-02-default-reek-configuration.md

+It's a combination of the [official `rails-friendly` configuration recommended in the Reek's README](https://github.com/troessner/reek?tab=readme-ov-file#working-with-rails) and some changes to the default rules.
+
+```yml
+detectors:


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

JuanVqz · 2025-09-01T18:44:04Z

2025-06-02-default-reek-configuration.md

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

FionaDL · 2025-09-05T02:40:40Z

2025-06-02-default-reek-configuration.md

+  TooManyStatements:
+    max_statements: 10 # original is 5
+  DuplicateMethodCall:
+    max_calls: 3 # original is 1


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)

FionaDL · 2025-09-05T02:48:52Z

2025-06-02-default-reek-configuration.md

+- Points
+
+Other project will not adapt this configuration. We expect Ruby and Rails projects to reach certain maturity before we adapt these standards.
+


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

mateusdeap · 2025-09-05T08:41:09Z

2025-06-02-default-reek-configuration.md

+    UnusedPrivateMethod:
+      enabled: false


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

mateusdeap · 2025-09-05T08:49:11Z

2025-06-02-default-reek-configuration.md

+
+When applying this configuration, projects will still need exceptions in some cases where the code smell is not flagging code we want to change or when there's a warning on code that's in the same file but out of the scope of the work done.
+
+It is expected that the team working on a given project will keep an eye when these exceptions are used, and there should be a clear explanation to skip a code smell instead of fixing it (which is a valid thing to do, but shouldn't be done just in order to not address it).


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

fbuys · 2025-09-10T15:16:04Z

2025-06-02-default-reek-configuration.md

+It's a combination of the [official `rails-friendly` configuration recommended in the Reek's README](https://github.com/troessner/reek?tab=readme-ov-file#working-with-rails) and some changes to the default rules.
+
+```yml
+detectors:


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

fbuys · 2025-09-10T15:47:11Z

2025-06-02-default-reek-configuration.md

+    public_methods_only: true # original is false
+
+directories: # reek's recommendation for Rails applications unless a comment is added
+  "app/controllers":


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.

fbuys · 2025-09-10T15:51:13Z

2025-06-02-default-reek-configuration.md

+    UtilityFunction:
+      enabled: false # complains about utility functions in generated migrations, it's ok to have utility functions in migrations
+
+exclude_paths:


- 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.

arielj added 2 commits June 2, 2025 16:46

DT-462: Add RFC for Reek configuration

2748514

DT-462: Updates based on feedback by amanda and ernesto

a1f61ce

fbuys reviewed Aug 28, 2025

View reviewed changes

JuanVqz reviewed Sep 1, 2025

View reviewed changes

FionaDL reviewed Sep 5, 2025

View reviewed changes

mateusdeap reviewed Sep 5, 2025

View reviewed changes

fbuys reviewed Sep 10, 2025

View reviewed changes

DT-462: add UncommunicativeVariableName exceptions

b73f255

		- Points

		Other project will not adapt this configuration. We expect Ruby and Rails projects to reach certain maturity before we adapt these standards.


		When applying this configuration, projects will still need exceptions in some cases where the code smell is not flagging code we want to change or when there's a warning on code that's in the same file but out of the scope of the work done.

		It is expected that the team working on a given project will keep an eye when these exceptions are used, and there should be a clear explanation to skip a code smell instead of fixing it (which is a valid thing to do, but shouldn't be done just in order to not address it).

DT-462: Add RFC for Reek configuration #5

Are you sure you want to change the base?

DT-462: Add RFC for Reek configuration #5

Conversation

arielj commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielj Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FionaDL Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielj Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielj Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielj Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

arielj commented Jun 2, 2025 •

edited

Loading

arielj Sep 15, 2025 •

edited

Loading

FionaDL Sep 5, 2025 •

edited

Loading

arielj Sep 5, 2025 •

edited

Loading

arielj Sep 5, 2025 •

edited

Loading

arielj Sep 5, 2025 •

edited

Loading