search

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

Inheriting measured from parent class #52

Open KevinVerre opened 6 years ago

KevinVerre commented 6 years ago

I ran into an issue today. I use single-table inheritance to create three subclasses for a single parent class. My original plan was to to call

measured_weight :weight

in the parent class file. I was hoping that would allow the objects of the children classes be able to accept a Measured:Weight. Instead it threw an error: measured_fields was nil and the error was that it was trying to call [ ] of a nil object.

The workaround was to add "measured_weight :weight" to each of the three subclasses.

Is this a known issue? Is there a way to fix it in the measured-rails gem?

kmcphillips commented 6 years ago

Hmmm... At some point I remember adding support for STI, but I don't see test cases covering it. It is not a known issue. But it should most definitely work.

Probably the bug would be in this part of the adapter: https://github.com/Shopify/measured-rails/blob/master/lib/measured/rails/active_record.rb

Would be super helpful if you could provide a failing test example.

kmcphillips commented 6 years ago

Wrote some failing tests on a branch: https://github.com/Shopify/measured-rails/compare/sti?expand=1

Is this what you're seeing?

NoMethodError:         NoMethodError: undefined method `[]' for nil:NilClass
            /home/kevin/source/measured-rails/lib/measured/rails/active_record.rb:59:in `block (2 levels) in measured'
kmcphillips commented 6 years ago

Can you try 1ca8759854f5a01056523f2e9feb4d8bcbd7f1e7 in your app and let me know if it solves your issue?

kmcphillips commented 6 years ago

If it does not, help me write more tests like in 5e0b149d67a12569b8bd2dcb363c7904856db7a9 to cover your case.

KevinVerre commented 6 years ago

@kmcphillips Yes, you correctly identified the error that I was seeing. active_record.rb:59. measured_fields was nil when I had the fields set in the parent class but tried to use it on a child object.

I looked at your two commits, I didn't try running them. You could add a test case for when the parent has one measured field and the child class has a different measured field.

KevinVerre commented 6 years ago

@kmcphillips but if all those tests pass it should probably work :)

kmcphillips commented 6 years ago

That's a good test case. I'll expand it. That's a pretty dicy requirement though.

kmcphillips commented 6 years ago

@KevinVerre Now that I'm thinking about this, I don't think I know what you mean:

You could add a test case for when the parent has one measured field and the child class has a different measured field.

Can you please give a clear example? Some code and/or requirements?

kmcphillips commented 6 years ago

Ping @KevinVerre ?

KevinVerre commented 6 years ago

@kmcphillips oh, it's not important. I was just imagining adding a test case like Kakapo < Bird < Animal. But then try to do complicated stuff with the measurements. So maybe....

Animal has an average weight. Bird has average weight and average wingspan Kakapo has average weight, average egg_weight

In this case Bird has a measured field that its child does not have in its class definition (wingspan), and Kakapo has a measured field that its parent class does not have in its class definition (egg_weight).