Shopify / measured-rails

Rails adapter for the measured gem. Encapsulate measurements and their units in Ruby and Rails.
MIT License
92 stars 13 forks source link

Include Support for Dynamic Subclass Attributes #65

Closed ericpowell03 closed 3 years ago

ericpowell03 commented 3 years ago

In my application, the category of unit (Volume, Length, Weight) is not known ahead of time so the column declarations cannot be used as designed. I would be helpful to be able to be able to declare a column of Measured type and have the getter return an instance of subclass based on the unit column, and perhaps a unit_type column where ambiguities exist (volume oz vs. weight oz.).

Here's an example of how it might look:

class Measurable < ApplicationRecord
  measured :unknown_measurement, 
           unit_field_name: :unknown_measurement_unit, 
           unknown_measurement_type_name: :unknown_measurement_type 
end

measurable = Measurable.new(unknown_value: 1, 
             unknown_measurement_unit: 'oz',
             unknown_measurement_type: 'Volume')

measurable.unknown_measurement
#<Measured::Volume: 1 #<Measured::Unit: oz (fl_oz, imp_fl_oz, imperial_fluid_ounce, imperial_fluid_ounces) 1/160 gal>> 
kmcphillips commented 3 years ago

Interesting.

I'd be interested to see a PR that attempts this. Happy to review or help.

This feels a little like a polymorphic association? There may be a different way to approach this. Because you here have to define the actual class unknown_measurement_type: as part of the creation API. So you can probably be clever with using that class in some other way. Might be better off putting in the right hooks into measured-rails to allow you to extend your specific use case for this feature.

ericpowell03 commented 3 years ago

Yes, agreed re similarity to a polymorphic relationship. I'm starting to explore this now and I'll definitely submit a PR if I can come up with something that generalizes well for the gem. I'll explore in more detail and let you know what questions I have. Thanks for the quick reply!

kmcphillips commented 3 years ago

Ok great. You can ping me by name if you have a draft PR up or want some feedback.

This gem is relatively simple compared to measured itself. Have a look in: https://github.com/Shopify/measured-rails/blob/master/lib/measured/rails/active_record.rb

I suspect this may not be easy to support though. As the class for the column is defined at load time of the model, and then it uses that to define accessors and some convenience methods, as well as store it in the column config. You'd need to defer all that, or maybe pass in some abstract superclass.

Anyway. Interested what you come up with.

ericpowell03 commented 3 years ago

Hey @kmcphillips - Just wanted to send you a quick update here. I ended up going a different direction on this. I'm using the measured gem still, just not the measured-rails adapter. You are welcome to leave the issue open, of course, but feel free to close it on my behalf. For others that may see this at some point, here's what I ended up doing to solve my particular issue (I'm also defining a few custom measured unit subclasses). Basically, I just defined custom getters/setters for the measured column:

# InventoryItem is a parent record that defines the "type"
# of item that is being tracked for inventory purposes.
# measured_by is a column that defines the measurement
# unit type that various child has_many records will delegate to it.
class InventoryItem < ApplicationRecord
  has_many :location_inventory_items, dependent: :destroy
  ...
validates :name, :measured_by, presence: true
  MeasuredBy = %w(Weight Volume Length Count Beer Wine Liquor)
  validates :measured_by, inclusion: { in: MeasuredBy}
  attr_readonly(:measured_by)
end

# This is a 'child' record of InventoryItem that has a 'par' value
# that is of type measured, with the subclass defined by its parent. 
class LocationInventoryItem < ApplicationRecord
  belongs_to :inventory_item
  belongs_to :location
  ...
  validates :par_unit, presence: true
  attr_accessor :par
  delegate :measured_by, to: :inventory_item

  def par
    raise "Measured By must be set on parent" if self.measured_by.nil?
    klass = self.measured_by.constantize
    klass.new(self.par_value, self.par_unit)
  end

  def par=(par)
    raise "Measured By must be set on parent" if self.measured_by.nil?
    klass = self.measured_by.constantize
    if par.is_a?(klass)
      self.par_value = par.value
      self.par_unit = par.unit.name
    elsif par.is_a?(Hash)
      self.par_value = par[:value]
      self.par_unit = par[:unit]
      klass.new(self.par_value, self.par_unit)
    else
      raise ArgumentError.new("Only instances of #{klass} or type hash allowed")
    end
  end

  private

  def set_par_unit
    if self.par_unit.nil?
      self.par_unit = self.inventory_item.default_unit
    end
  end
end
kmcphillips commented 3 years ago

That approach makes sense! This gem is considerably simpler and naive. It basically just is setters and getters, with some boxing and validation and syntactic sugar.

Thanks for the followup!