Skip to content

Conversation

p8
Copy link
Member

@p8 p8 commented Oct 31, 2023

This moves most of the implementation of ActiveRecord::Enum to
ActiveModel::Enum. This excludes:

  • scopes, as scopes aren't supported by ActiveModel.
  • method conflict detection, as this is difficult to implement in
    ActiveModel.
class Conversation
  include ActiveModel::Attributes
  include ActiveModel::Enum
  attribute :status, :integer
  enum :status, [ :active, :archived ]
end

conversation = Conversation.new
conversation.active!
conversation.active? # => true
conversation.status  # => "active"

The current implementation supports using integers for values and
strings for labels, if defined.

This also introduces ActiveSupport::Concern to ActiveRecord::Enum as
this allows both ActiveModel::Enum and ActiveRecord::Enum to be mixed in
with include instead of extend, like most other ActiveModel modules.

The tests were copied from ActiveRecord and modified for ActiveModel.
Scoping and method conflict detection tests were excluded.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@p8 p8 force-pushed the activemodel/enum branch 3 times, most recently from bfed7bd to d0a9a46 Compare November 1, 2023 10:46
@zzak
Copy link
Member

zzak commented Nov 4, 2023

cc @jonathanhefner and @ghiculescu who are probably interested in AM::Enum and would be good to get their feedback 🙏

@p8 p8 force-pushed the activemodel/enum branch 7 times, most recently from e031a7e to e5b884c Compare November 4, 2023 12:58
Allow ActiveRecord::Enum to be mixed in with include instead of
extend, like most other ActiveModel inherited modules.

This refactoring makes it easier to move `ActiveRecord::Enum` to
`ActiveModel::Enum` and mixin `ActiveModel::Enum` with include instead
of `extend`.
This moves most of the implementation of ActiveRecord::Enum to
ActiveModel::Enum. This excludes:

* scopes, as scopes aren't supported by ActiveModel.
* method conflict detection, as this is difficult to implement in
  ActiveModel.

```ruby
class Conversation
  include ActiveModel::Attributes
  include ActiveModel::Enum
  attribute :status, :integer
  enum :status, [ :active, :archived ]
end

conversation = Conversation.new
conversation.active!
conversation.active? # => true
conversation.status  # => "active"
```

The current implementation supports using integers for values and
strings for labels, if defined.

The tests were copied from ActiveRecord and modified for ActiveModel.
Scoping and method conflict detection tests were excluded.
@p8 p8 force-pushed the activemodel/enum branch from 8fa0ddb to 1007cf8 Compare November 4, 2023 13:24
@rafaelfranca
Copy link
Member

Thank you for the pull request. I didn't want Enum to be added to Active Record. For sure I'll not let it be added to Active Model. There is no reason to translate strings to integer in the active model layer. There is no benefits on doing that. I can see the benefits in Active Record.

@p8
Copy link
Member Author

p8 commented Nov 9, 2023

Hi @rafaelfranca,
You mentioned this at RailsWorld so I wasn't going to create this PR, but I saw Jonathan mentioning introducing ActiveModel::Enum in another PR so I thought I'd push my work 😄

I don't care about the mapping of integers to strings either. Keeping that in place just made the PR easier/smaller.

My main reason for an ActiveModel::Enum is to replace code like the following:

class SomeModel
  module States
    STARTED = 'started'
    DONE    = 'done'
    FAILED  = 'failed'
    ALL     = constants(false).map { |constant| const_get(constant) }
  end
  attribute :state, String
  validates :state, inclusion: { in: States::ALL }

  def started?
    state == States::STARTED
  end
  
  def done?
    state == States::DONE
  end
  
  def failed?
    state == States::FAILED
  end
  
  def state=(value)
    raise ArgumentError, "'#{value}' is not a valid state" unless States::ALL.include?(value)
    super
  end
end

With something like:

class SomeModel
  attribute :state, String
  enum :state, [:started, :done, :failed]
end

Would you be open to an ActiveModel::EnumType?
So we can do something like:

class SomeModel
  attribute :state, :enum, [:started, :done, :failed]
end

@rafaelfranca
Copy link
Member

My main reason for an ActiveModel::Enum is to replace code like the following:

That is exactly my problem with Enum. Look how many options he have to disable those methods. It is not like people always need them, and people sometimes don't want them. So not sure it is worth to add a a DSL to generate those methods given them aren't hard to implement.

A type is an interesting idea, but not sure it solve the problem you are trying to solve. Types don't generate methods. Neither do validation, they usually make invalid values nil.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants