rubocop-rails icon indicating copy to clipboard operation
rubocop-rails copied to clipboard

Consider `Rails.env` checks to be an anti-pattern.

Open ioquatix opened this issue 4 years ago • 4 comments

We would like to prevent something if Rails.env.production? type of code leaking into our code base. Specifically, we'd prefer that it's configuration driven, as in:

# bad
if Rails.env.production?
  add_job
end

# good
if Config.add_jobs?
  add_job
end

So we see Rails.env.(environment)? as an anti-pattern.

We have been trying out some proof-of-concept:

module Cop
  class EnvironmentCheck < RuboCop::Cop::Cop
    def_node_matcher :environment_check, <<~PATTERN
      (send (send (const nil? :Rails) :env) $_)
    PATTERN

    def on_send(node)
      environment_check(node) do |method|
        add_offense(node, message: message(method))
      end
    end

    private

    def message(method)
      "Using conditional logic like Rails.env.#{method} is an anti-pattern. Please replace with configuration specific to an environment."
    end
  end
end

ioquatix avatar Nov 22 '21 00:11 ioquatix

Can you explain a bit why Rails.env is an anti-pattern? To me this seems like a project-specific choice and to be honest, I don't see a reason to add this to rubocop-rails. However you can add a custom cop to your project.

tejasbubane avatar Nov 28 '21 18:11 tejasbubane

There was a problem recently on my current project. We were making checks for Rails.env.production? in data migrations. However, we use a different environment to run migrations, and it points to a production database. We ended up having records purposed for staging in our production database.

pirj avatar Nov 28 '21 18:11 pirj

This also relates to the Config aspect of The Twelve-Factor App pattern:

Another aspect of config management is grouping. Sometimes apps batch config into named groups (often called “environments”) named after specific deploys, such as the development, test, and production environments in Rails. This method does not scale cleanly: as more deploys of the app are created, new environment names are necessary, such as staging or qa. As the project grows further, developers may add their own special environments like joes-staging, resulting in a combinatorial explosion of config which makes managing deploys of the app very brittle.

https://12factor.net/config

andyw8 avatar Nov 28 '21 18:11 andyw8