Shopify / measured

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

Separate case insensitive concerns into separate class #53

Closed thegedge closed 7 years ago

thegedge commented 7 years ago

Problem

Right now, you have to pay some cost for case insensitive checks, even if you don't need them.

Solution

Remove this cost by creating a separate class to do downcase-ing. If a case-insensitive unit system is built, then this cost will be payed. If a case-sensitive unit system is built, no additional costs are paid.

Most of this PR is just a copy of unit_test.rb, with assertions adjusted for case insensitivity. rspec shared examples would be 👌

thegedge commented 7 years ago

Do we have adequate test coverage of the now complex case_sensitive() method? Meaning you're defending against a raise from a duplicate name. Do we test that DSL part?

Good catch! I forgot to add a test for that case. If we want to continue with having the DSL being declarative, I'll add some tests for that.

kmcphillips commented 7 years ago

So. Is there a better way of representing case_sensitive? A better place to put it or express it?

thegedge commented 7 years ago

So. Is there a better way of representing case_sensitive? A better place to put it or express it?

Good question. Personally, I'd rather drop this entirely from the library and have the clients downcase when / if necessary.

Is there a better place? Probably not. I feel like the only other option is what we had before my changes. Open to suggestions though 😄

As mentioned, if we do keep it here, I can remove the declarative-ness of the DSL by requiring case_sensitive first (or maybe as an option to build?). This will actually simplify the code a bunch by removing the need to rebuild the units list in the conversion builder.

kmcphillips commented 7 years ago

I really like .build(case_sensitive: true)

thegedge commented 7 years ago

I really like .build(case_sensitive: true)

It definitely simplifies the builder to do it this way, so I pushed a change to do so. Previous test coverage should be sufficient now.

thegedge commented 7 years ago

^ Squashed and reworded the commits.