rubocop / rubocop-rails

A RuboCop extension focused on enforcing Rails best practices and coding conventions.
https://docs.rubocop.org/rubocop-rails
MIT License
802 stars 257 forks source link

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

Open ioquatix opened 2 years ago

ioquatix commented 2 years ago

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
tejasbubane commented 2 years ago

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.

pirj commented 2 years ago

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.

andyw8 commented 2 years ago

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

lulalala commented 2 months ago

Not sure if this is a valid case: we had a return if Rails.env.development? logic deep in our call. When I changed a part of my code, this part of the code was never executed locally. We only found it would cause issues on production.