From 274851470402be5fe6997150090d597d1f6e39f5 Mon Sep 17 00:00:00 2001 From: Ariel Juodziukynas Date: Mon, 2 Jun 2025 16:46:59 -0300 Subject: [PATCH 1/4] DT-462: Add RFC for Reek configuration --- 2025-06-02-default-reek-configuration.md | 87 ++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 2025-06-02-default-reek-configuration.md diff --git a/2025-06-02-default-reek-configuration.md b/2025-06-02-default-reek-configuration.md new file mode 100644 index 0000000..7526dde --- /dev/null +++ b/2025-06-02-default-reek-configuration.md @@ -0,0 +1,87 @@ +# Default Reek Configuration + +## The Problem + +We like to use Reek in many of our projects as one of the tools to aid in improving the code quality, but we do not have a clear expected way to use Reek in the different projects we use and we have no clear guidelines about when to make exception or changes to its configuration. + +## Proposed initial Reek configuration + +This RFC is a proposal of a default Reek configuration file that we would use in any new project to reduce the number of warnings and the need of exceptions that we see with the [default configuration](https://github.com/troessner/reek/blob/a8649370ced77dd0e798b00dc97ff916f68bb002/docs/defaults.reek.yml), and to also define some guideline or expectation on how we can decide when to add exceptions and modifications for different needs. + +### Default .reek configuration file + +This file is meant to be added in the root of the project as `.reek.yml` to handle common cases of Reek warnings that will want either disabled or reconfigured based on what we've seen in many projects. + +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: + IrresponsibleModule: + enabled: false # we don't really add module descriptions ever + TooManyStatements: + max_statements: 10 # original is 5 + DuplicateMethodCall: + max_calls: 3 # original is 1 + NilCheck: + enabled: false # in some projects were this was enabled, it was a false positive being ignored + UnusedPrivateMethod: + enabled: true # default is false, we are still disabling it for controllers and models below + UtilityFunction: + public_methods_only: true # original is false + +directories: # reek's recommendation for Rails applications unless a comment is added + "app/controllers": + NestedIterators: + max_allowed_nesting: 2 + UnusedPrivateMethod: + enabled: false + InstanceVariableAssumption: + enabled: false + TooManyInstanceVariables: + enabled: false # instance variables are the way to pass data to views, it's expected + "app/helpers": + UtilityFunction: + enabled: false + "app/mailers": + InstanceVariableAssumption: + enabled: false + "app/models": + InstanceVariableAssumption: + enabled: false + UnusedPrivateMethod: + enabled: false + "db/migrate": + FeatureEnvy: + enabled: false # keeps complaining about `t.string :col_name` as feature envy calling `t` more than `self` + UncommunicativeVariableName: + enabled: false # complains `t` is uncommunicative, but it's a common short name for `table` + TooManyStatements: + enabled: false # long tables require many statements, reek keeps complaining about them + DuplicateMethodCall: + enabled: false # complains about duplicated methods when a migration creates more than 1 table, which cannot be "fixed" + UtilityFunction: + enabled: false # complains about utility functions in generated migrations, it's ok to have utility functions in migrations + +exclude_paths: + - node_modules # in case any node package has a .rb file (like the styleguide) + - vendor # in case gems are bundle inside the project's folder vendor folder +``` + +### Exceptions and :reek:\* Comments + +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). + +## What this file is NOT + +There will never be a single Reek configuration file to match the needs of all projects we have, this file is only a starting point with some known generic settings. + +## What we are looking as comments + +For this RFC we are expecting comments of different types: + +- if you disagree with any of the proposed defaults, let us know +- if you think another setting that is not listed here should have a different config, let us know +- if you think it's not clear when a code smell can be skipped and you have some examples from public projects, we can try to discuss and improve the guidelines on when to skip them with links +- if you think any other common rails directory should have different rules, let us know From a1f61ce942d42e90fe8e32ee74486962cacaab7b Mon Sep 17 00:00:00 2001 From: Ariel Juodziukynas Date: Fri, 6 Jun 2025 14:20:06 -0300 Subject: [PATCH 2/4] DT-462: Updates based on feedback by amanda and ernesto --- 2025-06-02-default-reek-configuration.md | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/2025-06-02-default-reek-configuration.md b/2025-06-02-default-reek-configuration.md index 7526dde..f06bf7f 100644 --- a/2025-06-02-default-reek-configuration.md +++ b/2025-06-02-default-reek-configuration.md @@ -6,7 +6,7 @@ We like to use Reek in many of our projects as one of the tools to aid in improv ## Proposed initial Reek configuration -This RFC is a proposal of a default Reek configuration file that we would use in any new project to reduce the number of warnings and the need of exceptions that we see with the [default configuration](https://github.com/troessner/reek/blob/a8649370ced77dd0e798b00dc97ff916f68bb002/docs/defaults.reek.yml), and to also define some guideline or expectation on how we can decide when to add exceptions and modifications for different needs. +This RFC is a proposal of a default Reek configuration file that we would use in established projects to reduce the number of warnings and the need of exceptions that we see with the [default configuration](https://github.com/troessner/reek/blob/a8649370ced77dd0e798b00dc97ff916f68bb002/docs/defaults.reek.yml), and to also define some guideline or expectation on how we can decide when to add exceptions and modifications for different needs. ### Default .reek configuration file @@ -73,6 +73,24 @@ When applying this configuration, projects will still need exceptions in some ca 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). +## How is this going to impact our internal teams and their day to day? + +In the near future we plan to adapt this configuration in these projects: + +- FastRuby.io +- OmbuLabs.com +- Points + +Other project will not adapt this configuration. We expect Ruby and Rails projects to reach certain maturity before we adapt these standards. + +## What projects are not a good fit for this new standard? + +We do NOT want to adapt this standard on these types of projects: + +- Prototypes +- Unproven MVPs that might be discarded +- One-off internal tools that might be obsolete in a year + ## What this file is NOT There will never be a single Reek configuration file to match the needs of all projects we have, this file is only a starting point with some known generic settings. From b73f255943b12c607b58ec7c0dde07af271b2416 Mon Sep 17 00:00:00 2001 From: Ariel Juodziukynas Date: Mon, 15 Sep 2025 12:50:02 -0300 Subject: [PATCH 3/4] DT-462: add UncommunicativeVariableName exceptions --- 2025-06-02-default-reek-configuration.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/2025-06-02-default-reek-configuration.md b/2025-06-02-default-reek-configuration.md index f06bf7f..ad0bd96 100644 --- a/2025-06-02-default-reek-configuration.md +++ b/2025-06-02-default-reek-configuration.md @@ -24,6 +24,11 @@ detectors: max_calls: 3 # original is 1 NilCheck: enabled: false # in some projects were this was enabled, it was a false positive being ignored + UncommunicativeVariableName: + accept: + - e + - x + - _ UnusedPrivateMethod: enabled: true # default is false, we are still disabling it for controllers and models below UtilityFunction: From 9945f2c309a3c18850e9d0eb1ae4b28299bb507f Mon Sep 17 00:00:00 2001 From: Ariel Juodziukynas Date: Mon, 13 Oct 2025 15:46:11 -0300 Subject: [PATCH 4/4] DT-462: address some feedback of the RFC --- 2025-06-02-default-reek-configuration.md | 42 +++++++++++------------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/2025-06-02-default-reek-configuration.md b/2025-06-02-default-reek-configuration.md index ad0bd96..ead53a7 100644 --- a/2025-06-02-default-reek-configuration.md +++ b/2025-06-02-default-reek-configuration.md @@ -16,19 +16,21 @@ It's a combination of the [official `rails-friendly` configuration recommended i ```yml detectors: - IrresponsibleModule: - enabled: false # we don't really add module descriptions ever - TooManyStatements: - max_statements: 10 # original is 5 DuplicateMethodCall: max_calls: 3 # original is 1 + IrresponsibleModule: + enabled: false # we don't really add module descriptions ever NilCheck: enabled: false # in some projects were this was enabled, it was a false positive being ignored + TooManyStatements: + max_statements: 10 # original is 5 UncommunicativeVariableName: accept: - - e - - x - - _ + - e # common in `rescue => e` (short for `exception`) + - x # common in one-liners + - _ # common when ignoring a parameter + - k # common when looping through hashes (short for `key`) + - v # common when looping through hashes (short for `value`) UnusedPrivateMethod: enabled: true # default is false, we are still disabling it for controllers and models below UtilityFunction: @@ -36,14 +38,16 @@ detectors: directories: # reek's recommendation for Rails applications unless a comment is added "app/controllers": - NestedIterators: - max_allowed_nesting: 2 - UnusedPrivateMethod: - enabled: false InstanceVariableAssumption: enabled: false + NestedIterators: + max_allowed_nesting: 2 TooManyInstanceVariables: enabled: false # instance variables are the way to pass data to views, it's expected + TooManyStatements: + enabled: false + UnusedPrivateMethod: + enabled: false "app/helpers": UtilityFunction: enabled: false @@ -53,21 +57,13 @@ directories: # reek's recommendation for Rails applications unless a comment is "app/models": InstanceVariableAssumption: enabled: false + TooManyStatements: + enabled: false UnusedPrivateMethod: enabled: false - "db/migrate": - FeatureEnvy: - enabled: false # keeps complaining about `t.string :col_name` as feature envy calling `t` more than `self` - UncommunicativeVariableName: - enabled: false # complains `t` is uncommunicative, but it's a common short name for `table` - TooManyStatements: - enabled: false # long tables require many statements, reek keeps complaining about them - DuplicateMethodCall: - enabled: false # complains about duplicated methods when a migration creates more than 1 table, which cannot be "fixed" - UtilityFunction: - enabled: false # complains about utility functions in generated migrations, it's ok to have utility functions in migrations exclude_paths: + - db/migrate - node_modules # in case any node package has a .rb file (like the styleguide) - vendor # in case gems are bundle inside the project's folder vendor folder ``` @@ -86,7 +82,7 @@ In the near future we plan to adapt this configuration in these projects: - OmbuLabs.com - Points -Other project will not adapt this configuration. We expect Ruby and Rails projects to reach certain maturity before we adapt these standards. +Other project will not adapt this configuration. We expect Ruby and Rails projects to reach certain maturity before we adapt these standards. Ops will decide when a project has reached this maturity. ## What projects are not a good fit for this new standard?