Clean up tests and formatting for Location #446

kmcphillips · 2016-12-13T21:29:14Z

All cleanup in Location.

No functional change. Cleans up formatting, extracts a constant, removes old hash syntax, expands methods to be more consistent.

Reformats the tests to be test "" do and adds a bunch of missed coverage.

jonathankwok · 2016-12-14T13:12:54Z

lib/active_shipping/location.rb

      hash_access = begin
        object[:some_symbol]
        true
      rescue
        false
      end
-      attr_mappings.each do |pair|
+
+      ATTRIBUTE_MAPPINGS.each do |pair|


While we're here, we can make this a little clearer to read too. What do you think about these changes?

Renaming ATTRIBUTE_MAPPINGS to ATTRIBUTE_ALIASES

Change the each pair parameter to an actual pair of named parameters, attribute and aliases:

Change the pair[1].each to be a detect since that's what we're really doing anyway with the break.

So something like:

ATTRIBUTE_ALIASES.each do |attribute, aliases| aliases.detect do |alias| # blah blah

It makes it abundantly clear that we're assigning the aliased attribute's value to whichever aliased attribute what passed in for that attribute.

I just noticed the if value = (object[sym] if hash_access) gross.

Yeah, mentioned it offline but object.send(sym) if object.respond_to?(sym) should be synonymous to object.try(sym) in this case.

I don't know why it's so complicated, value = hash_access ? object[sym] : object.try(sym) should actually do what we want - use hash access if possible, otherwise use instance method if applicable.

I think that makes it much more readable too.

jonathankwok · 2016-12-14T13:30:45Z

lib/active_shipping/location.rb

@@ -129,9 +149,9 @@ def inspect
    # Returns the postal code as a properly formatted Zip+4 code, e.g. "77095-2233"
    def zip_plus_4
      if /(\d{5})(\d{4})/ =~ @postal_code
-        return "#{$1}-#{$2}"


We can simplify this, I believe;

def zip_plus_4 "#{$1}-#{2} if /(\d{5})-?(\d{4})/ =~ @postal_code end

Tested it out a bit:

irb(main):001:0> "#{$1}-#{$2}" if /(\d{5})-?(\d{4})/ =~ "12345-1234" => "12345-1234" irb(main):002:0> "#{$1}-#{$2}" if /(\d{5})-?(\d{4})/ =~ "123451234" => "12345-1234" irb(main):003:0> "#{$1}-#{$2}" if /(\d{5})-?(\d{4})/ =~ "K2P 1L4" => nil

thegedge

👍 after addressing @jonathankwok's comments

thegedge · 2016-12-14T14:56:47Z

lib/active_shipping/location.rb

-        :address_type => [:address_type],
-        :company_name => [:company, :company_name]
-      }
+      return object if object.is_a?(ActiveShipping::Location)


Something to consider for a future PR: perhaps this should dupe the object? I feel like a from method should return something I can mutate without having side effects somewhere else.

thegedge · 2016-12-14T15:13:49Z

test/unit/location_test.rb

-    location = location_fixtures[:ottawa]
-    assert !location.commercial?
+  test "#address_type= assigns a type of address as commercial" do
+    refute @location.commercial?


We should switch to assert_predicate here and everywhere else. Okay to leave this for a future PR, but best to create an issue if deferring. Feel free to assign to me.

kmcphillips · 2016-12-14T16:42:29Z

Reping with changes.

kmcphillips · 2016-12-14T16:44:00Z

test/unit/location_test.rb

+    assert_equal @attributes_hash[:name], location.name
+  end
+
+  test ".from sets from an object with properties" do


We didn't actually have any tests for .from an object, not a hash.

This Class.new is a little nasty, but I tried with OpenStruct and it still responds to :[] so doesn't work.

kmcphillips · 2016-12-14T16:46:26Z

lib/active_shipping/location.rb

-            break
+
+      ATTRIBUTE_ALIASES.each do |attribute, aliases|
+        aliases.detect do |sym|


This is a little weird in that detect we don't care about the return value, but we do want it to exit early. Test fails if you change it to each.

jonathankwok · 2016-12-14T17:24:28Z

lib/active_shipping/location.rb

+        aliases.detect do |sym|
+          value = if hash_access
+            object[sym]
+          elsif object.respond_to?(sym) && !Hash.public_instance_methods.include?(sym.to_s)


I'm trying to understand what the second check there is for. Two questions:

.public_instance_methods returns an array of symbols, so the include would always be false and result in this predicate always being true.

"Call this method only if Hash objects don't have it"? What does that give us? None of the aliased symbols are instance methods on Hash.

Yes. You're of course right.

kmcphillips · 2016-12-14T17:37:50Z

Good @jonathankwok ?

kmcphillips · 2016-12-14T19:32:41Z

Oops, didn't squash because I'm a chuuuuuuump... 😣

jonathankwok · 2016-12-14T19:34:38Z

At least they're not goofy commit names 😛

Clean up tests and formatting for Location

Clean up tests and formatting for Location

548065d

kmcphillips added this to the Version 2.0 milestone Dec 13, 2016

kmcphillips requested review from thegedge and jonathankwok December 13, 2016 21:29

jonathankwok suggested changes Dec 14, 2016

View reviewed changes

thegedge approved these changes Dec 14, 2016

View reviewed changes

kmcphillips commented Dec 14, 2016

View reviewed changes

kmcphillips added 2 commits December 14, 2016 11:45

Code review: Refactor alias lookups and add tests for .from

c4aa010

Code review: Simplify zip regex

8e01d09

kmcphillips force-pushed the modernize-location-tests branch from b010ed9 to 8e01d09 Compare December 14, 2016 16:45

kmcphillips commented Dec 14, 2016

View reviewed changes

jonathankwok reviewed Dec 14, 2016

View reviewed changes

Code review: Unnecessary hash check

68b519c

jonathankwok approved these changes Dec 14, 2016

View reviewed changes

kmcphillips merged commit dd35f38 into master Dec 14, 2016

kmcphillips deleted the modernize-location-tests branch December 14, 2016 19:31

garethson had a problem deploying to rubygems May 30, 2017 16:01 Failure

garethson had a problem deploying to rubygems May 30, 2017 18:03 Failure

garethson had a problem deploying to rubygems May 30, 2017 18:07 Failure

garethson deployed to rubygems May 30, 2017 18:14 Active

maartenvg pushed a commit that referenced this pull request Nov 9, 2017

Merge pull request #446 from Shopify/modernize-location-tests

acaf82b

Clean up tests and formatting for Location

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up tests and formatting for Location #446

Clean up tests and formatting for Location #446

kmcphillips commented Dec 13, 2016

jonathankwok Dec 14, 2016

kmcphillips Dec 14, 2016

jonathankwok Dec 14, 2016

jonathankwok Dec 14, 2016

jonathankwok Dec 14, 2016

thegedge left a comment

thegedge Dec 14, 2016

thegedge Dec 14, 2016

kmcphillips Dec 14, 2016

kmcphillips commented Dec 14, 2016

kmcphillips Dec 14, 2016

kmcphillips Dec 14, 2016

jonathankwok Dec 14, 2016

kmcphillips Dec 14, 2016

kmcphillips Dec 14, 2016

kmcphillips commented Dec 14, 2016

kmcphillips commented Dec 14, 2016

jonathankwok commented Dec 14, 2016

Clean up tests and formatting for Location #446

Clean up tests and formatting for Location #446

Conversation

kmcphillips commented Dec 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thegedge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmcphillips commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmcphillips commented Dec 14, 2016

kmcphillips commented Dec 14, 2016

jonathankwok commented Dec 14, 2016