search

Shopify / measured

Encapsulate measurements and their units in Ruby and Ruby on Rails.
MIT License
337 stars 28 forks source link

Revert case_sensitive #22

Closed kmcphillips closed 9 years ago

kmcphillips commented 9 years ago

Sorry, @AnthonyBobsin but I am going to have to revert this for now. I should have done a better job reviewing it. But there are a couple of problems:

1) The case sensitivity should be defined at a system level. You shouldn't be able to say "centimetres are case sensitive but kilometers are not". It should be "all measurements of length are case insensitive".

2) Some of the comparison methods had no tests for nil/blank and were causing exceptions when I ran the tests in `measured-rails.

I'm reverting your merge, pulled your copy changes back in, bumped to 1.1.0 as I had previously released 1.0.0, and we can work at revising your feature.

Sorry again for not being more diligent about the review and feedback.

cc @garethson

garethson commented 9 years ago

Great feature! But I agree with your points.

:ship:

kmcphillips commented 9 years ago

Revert of #20

AnthonyBobsin commented 9 years ago

Sounds good, thank you for giving me feedback. Not sure how I missed the tests for nil/blank, sorry about that. I'll work on the problems you pointed out and submit another MR when ready :+1:

kmcphillips commented 9 years ago

No problem! Looked good to me, but we have tests in measured-rails which test the nil cases. Shows shortcomings in the tests here in general. Thanks.

kmcphillips commented 9 years ago

FYI

  1) Error:

Measured::Rails::ValidationTest#test_validation_fails_if_only_only_the_value_is_set:
NoMethodError: undefined method `downcase' for nil:NilClass
    /home/travis/.rvm/gems/ruby-2.1.0/gems/measured-1.0.0/lib/measured/unit.rb:58:in `map'
    /home/travis/.rvm/gems/ruby-2.1.0/gems/measured-1.0.0/lib/measured/unit.rb:58:in `with_case_sensitivity'
    /home/travis/.rvm/gems/ruby-2.1.0/gems/measured-1.0.0/lib/measured/unit.rb:19:in `names_include?'
    /home/travis/.rvm/gems/ruby-2.1.0/gems/measured-1.0.0/lib/measured/conversion.rb:27:in `block in unit_or_alias?'
    /home/travis/.rvm/gems/ruby-2.1.0/gems/measured-1.0.0/lib/measured/conversion.rb:27:in `each'
    /home/travis/.rvm/gems/ruby-2.1.0/gems/measured-1.0.0/lib/measured/conversion.rb:27:in `unit_or_alias?'
    /home/travis/.rvm/gems/ruby-2.1.0/gems/measured-1.0.0/lib/measured/measurable.rb:88:in `valid_unit?'
    /home/travis/build/Shopify/measured-rails/lib/measured/rails/active_record.rb:66:in `block (2 levels) in measured'
    /home/travis/build/Shopify/measured-rails/test/validation_test.rb:90:in `block in <class:ValidationTest>'

https://travis-ci.org/Shopify/measured-rails/builds/75272946