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

TimeZoneAssignment should allow ActiveSupport::CurrentAttributes

Open nevans opened this issue 1 year ago • 0 comments

Is your feature request related to a problem? Please describe.

I like it when rubocop and rubocop-rails remind me about (or informs me of) modern best practices. One such best practice is using ActiveSupport::CurrentAttributes for specific limited forms of global context. This provides an isolated attributes singleton, which resets automatically before and after each request. It is based on Thread or Fiber local variables, and can also use Fiber.storage (introduced in ruby 3.2) to fix a variety of fiber-related bugs.

See the official rails documentation example here: https://api.rubyonrails.org/classes/ActiveSupport/CurrentAttributes.html

The simplest example of using this that is relevant to an already existing cop is the assignment of Time.zone.

Describe the solution you'd like

After adding a specific exception for ActiveSupport::CurrentAttributes, I would like to see the TimeZoneAssignment cop enabled in all ruby files, by default.

Rails/TimeZoneAssignment should allow direct assignment of Time.zone in a class that inherits from ActiveSupport::CurrentAttributes. However, when that is detected, it should require Time.zone to be also set to nil or some other default value inside a resets block. For example:

# bad
Time.zone = 'EST'

# good
Time.use_zone('EST') do
end

# good
class Current < ActiveSupport::CurrentAttributes
  attribute :user

  resets do
    Time.zone = nil
  end

  def user=(user)
    super
    Time.zone = user&.time_zone
  end
end

Describe alternatives you've considered

Continue as-is, only enabled for spec/**/*.rb and test/**/*.rb.

Alternatively, this rule could be enabled everywhere and only exclude app/models/current.rb by default.

Additional context

An additional cop should be added to require CurrentAttributes assignment is either done in a before_action controller callback, or using a block with Current.set.

nevans avatar May 08 '23 23:05 nevans