Implement automatic predicate method generation for ActiveHash::Enum

[hatsu38]

close #319

Description

This PR addresses the issue of repetitive predicate method definitions in ActiveHash when using enum types. It implements an automatic generation of status check methods based on the defined enum values, simplifying the usage of ActiveHash::Enum.

Changes

• Added define_enum_methods to ActiveHash::Enum::Methods module
• Automatically generate predicate methods when enum_accessor is called
• Sanitize method names by removing non-word characters and leading/trailing underscores
• Define instance methods that compare record IDs for enum value checking

Example

Before:

class PublicStatus < ActiveHash::Base
  include ActiveHash::Enum
  self.data = [
    { id: 1, name: "Publish", type: "published" },
    { id: 2, name: "Draft", type: "drafted" },
    { id: 3, name: "Archive", type: "archived" }
  ]
  enum_accessor :type

  def published?
    id == PublicStatus::PUBLISHED.id
  end

  def drafted?
    id == PublicStatus::DRAFTED.id
  end

  def archived?
    id == PublicStatus::ARCHIVED.id
  end
end

After:

class PublicStatus < ActiveHash::Base
  include ActiveHash::Enum
  self.data = [
    { id: 1, name: "Publish", type: "published" },
    { id: 2, name: "Draft", type: "drafted" },
    { id: 3, name: "Archive", type: "archived" }
  ]
  enum_accessor :type
  # No need to manually define published?, drafted?, archived? methods
end

status = PublicStatus.find(1)
status.published? # => true
status.drafted?   # => false
status.archived?  # => false

Testing

Added unit tests to verify the automatic generation of predicate methods
Tested with various enum value names to ensure proper sanitization

[flavorjones]

I think we need to make this opt-in behavior (and not default behavior) because people may have already implemented their own methods that, depending on when the Enum module is included, might be clobbered during an upgrade.

I'd love to hear your thoughts on how to handle that case.

[flavorjones]

Did this subexpression come from somewhere else? Why not use something like Active Support's #camelize instead of this?

Also, how is this implementation going to handle collisions (e.g., two values like "FooBar" and "foo_bar" that would create the same method name)? How does rails handle collisions? I would like to see some test coverage for a collision case as well

[hatsu38]

The enum_value was derived from this section.

active_hash/lib/enum/enum.rb

Lines 48 to 49 in a192385

	constant.gsub!(/\W+/, "_")
	constant.gsub!(/^_|_$/, '')

Also, how is this implementation going to handle collisions (e.g., two values like "FooBar" and "foo_bar" that would create the same method name)?

I try it.
A DuplicateEnumAccessor error occurs when the set_constant method is executed.

active_hash/lib/enum/enum.rb

Line 39 in a192385

raise DuplicateEnumAccessor, "#{constant} already defined for #{self.class}" unless const_get(constant, false) == record

And We have also added a test for cases where the method is already defined by itself.
00c1497

[flavorjones]

@kbrock wdyt of this feature

[kbrock]

@flavorjones I had started to throw some code together to see how I would implement it. I'm glad this PR came through before I got too far.

Not really a fan of this, but I appreciate that we all have our own preferred styles.

https://api.rubyonrails.org/v5.1/classes/ActiveRecord/Enum.html

Docs have:

class Conversation < ActiveRecord::Base
  enum status: [ :active, :archived ]
end

conversation.active? # => true
conversation.status  # => "active"

A quick implementation would probably be like:
(think they have some code cache type framework to make this a little easier)

def enum columns
  methods = []
  columns.each do |column, values|
    values.each do |value|
      methods << "def #{value}? ; #{column} == #{value.inspect} ; end"
    end
  end
  instance_eval { methods.uniq.join(";") }
end

So then that begs to ask, why do we define a constant for each class in enum? This feels very different from active record.
This new request is more similar, though I'd just compare the column value to the constant rather than dynamically adding values for each record.

I want to accept this, but feel my negativity coming through. I do not understand why I'm negative though. It seems relatively sound.

Tangent: the while undefining a constant when the field is removed distracts me. Don't think we should be supporting delete/insert/update type behavior. again, not related though

[hatsu38]

Thank you for the insightful comment!

Don't think we should be supporting delete/insert/update type behavior. again, not related though

I agree with this.

I personally prefer the following style, where when calling enum_accessor, methods like drafted? are also available. This is because I often use enum_accessor.

class PublicStatus < ActiveHash::Base
  include ActiveHash::Enum
  self.data = [
    { id: 1, name: "Publish", type: "published" },
    { id: 2, name: "Draft", type: "drafted" },
    { id: 3, name: "Archive", type: "archived" }
  ]
  enum_accessor :type
  # No need to manually define published?, drafted?, archived? methods
end

Do you prefer the approach of not using enum_accessor, like the one below?

class PublicStatus < ActiveHash::Base
  include ActiveHash::Enum
  self.data = [
    { id: 1, name: "Publish", type: "published" },
    { id: 2, name: "Draft", type: "drafted" },
    { id: 3, name: "Archive", type: "archived" }
  ]

  enum type: [:published, :drafted, :archived]
end

I'm currently debating the implementation approach.

[kbrock]

@hatsu38 Thank you for your logical thoughts.

-  enum_accessor :type
+  enum type: [:published, :drafted, :archived]

• I like the first because you have less duplication.
• I like the second because you can easily include that line before and after the data is set.
• I like the second because it looks more similar to active record.

The current implementation to add constants is a little troubling. I don't like that it keeps adding constants to the class for every record added. I would like to avoid adding different methods to the class for every record added. I'd prefer to make as many changes all at once.

If we agree that enum must be defined after data, then enum :type, :type2 also seems acceptable. Not sure if documentation will help people avoid mistakes or if it is inviting support issues.

[hatsu38]

@kbrock Thanks you!

The implementation with

enum type: [:published, :drafted, :archived]

is closer to Rails, making it easier to understand for those who are new to ActiveHash. After actually trying it out, it feels pretty good to write! I think I'll proceed with this approach 🙆

[kbrock]

Also, though my opinion has changed over the years, I think I'm in camp instance_eval {} vs def_method(). All those closures hanging about makes me nervous.

[hatsu38]

There are cases where I want to use uppercase letters in the enum values.

To support this, I'd like to implement functionality similar to Rails' enum, which allows for both simple array definitions and more complex hash-based definitions.

For example, I want to support both of these use cases:

enum status: [:draft, :published, :archived]

enum action: { like: 'LIKE', comment: 'COMMENT', follow: 'FOLLOW', mention: 'MENTION' }

This approach would provide greater flexibility in defining enums, similar to how it's done in Active Record (as documented here: https://api.rubyonrails.org/v7.1/classes/ActiveRecord/Enum.html).

[kbrock]

good find. this looks great

[hatsu38]

Also, though my opinion has changed over the years, I think I'm in camp instance_eval {} vs def_method(). All those closures hanging about makes me nervous.

I wasn't sure how to use instance_eval, so I tried using class_eval instead.
If you notice anything that could be improved, I'd appreciate your feedback in the review! 👍

[kbrock]

Suggested change

	method_definitions << "def #{method}?; #{column} == '#{value}'; end"
	method_definitions << "def #{method}?; #{column} == #{value.inspect}; end"

Will value.inspect work?
I'd prefer to use that if it buys us enumeration with columns that contain integers.

Strings are most important, integers are a nice to have, and symbols are a very distant nice to have. I think we get all 3 with inspect

UPDATE:
Looking at your referenced link, I see that they tried to cover numbers and symbols as well.

[hatsu38]

I have decided to go with your suggestion!
5cab8ac

bundle exec rspec spec/enum/enum_spec.rb   
...........

Finished in 0.01806 seconds (files took 0.49792 seconds to load)
11 examples, 0 failures

[kbrock]

This looks great.

I tried do an injection attack here and was not able to get anything through.
(I was able to sneak in one for '#{value}')

The closes I can come is:

# this returns true for all values:

x="a'||true||'"
instance_eval("def a; 'z' == '#{x}' ; end") ; a
# => true

# this creates a file named yy

"x' ; `touch yy` ; '"

instance_eval("def a; 'z' == '#{x}' ; end") ; a
# => true

# not able to exploit `inspect` but still looking a way to inject something.

x=:"a||true"
instance_eval("def a; 'z' == #{x.inspect} ; end") ; a
# => false

@flavorjones you good to go on this one?

[flavorjones]

I've pushed a commit that sets the line number for eval and uses frozen strings -- these are Rails project conventions that I think are good.

I've also rebased onto master.

[flavorjones]

Whoops, force-pushed a fix to the frozen strings comment.

[kbrock]

Great call on fronzen_string_literal and the line number.

I had taken my inspiration from https://github.com/rails/rails/blob/main/activesupport/lib/active_support/core_ext/class/attribute.rb#L119-L120

They only call class_eval once, hence the method_definitions, and they use the caller location. But this should work fine.

Interestingly, enum uses def_method.

^ more of a note to future self.