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]

@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]

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]

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]

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