Change Package interface for ActiveShipping 2.0 #469

jonathankwok · 2017-01-12T23:17:20Z

Preface

This PR makes major opinionated changes to the interface of ActiveShipping::Package. Feel free to chime in with your own opinions, because these opinions are often my own and I've been known to be wrong in the past. 😛

Changes

There are many changes here. Here's a list, with some more explicit details below each.

The constructor method for Package is much more explicit, at the cost of an increase in verbosity.
- No more memorization of argument order - length:, width:, height:, weight:, options: in any order. Keyword arguments!
The only dimension that can be omitted is Height.
- Omitting any dimension raises an ArgumentError except for Height, which is commonly omitted in the shipping industry (e.g. envelopes!). In this case, it'll default to Measured::Length(0, :cm), where the unit is the same as the defined dimensions' units.
units/weight_unit_system/dimensions_unit_system/all the unit things are gone. We're using Measured objects so we shouldn't need them.
- It also means you can mix and match your units willy-nilly. If you want to have a length in centimetres and a width in inches, go wild.
The named unit methods like ounces, pounds, centimetres, inches, etc are all gone.
- I've introduced a dimensions method that operates like the old inches/centimetres methods, returning an array of the dimensions; only in Measured objects, not Floats.
- ounces and pounds were mostly just arbitrary shortcuts to converting Measured objects and getting their raw values. I don't believe it's necessary but I can be persuaded otherwise.
Volumetric and dimensional weight calculations are removed, because they're calculated on a per-carrier basis, not generically.
- See: Refactor and remove the weight/measurement methods with params on Package and PackageItem #466 (comment)

This is incomplete!

There's a few things that need to be done before this can be merged.

Fix everything that breaks because of the interface change. Like 40% of our tests are now red.
Figure out a fix for cents_from. Related: Fix money parsing #458
Reintroduce volumetric/dimensional weight. Probably the same way we do 2.
Add Measured 2.0 so we can actually use the new Measured features.

kmcphillips

This is really really good.

A number of style comments. A couple bugs, that you should expand tests to cover.

But in general I strongly agree with this new interface. So much cleaner, simpler, and easier to understand.

kmcphillips · 2017-01-13T02:16:34Z

lib/active_shipping/package.rb

@@ -1,118 +1,62 @@
 module ActiveShipping #:nodoc:
  class Package
-    cattr_accessor :default_options
-    attr_reader :options, :value, :currency
+    cattr_accessor :default_options do {} end


Prefer:

cattr_accessor: default_options self.default_options = {}

kmcphillips · 2017-01-13T02:19:27Z

lib/active_shipping/package.rb

-      @oversized = options[:oversized] ? true : false
-      @unpackaged = options[:unpackaged] ? true : false
+    def dimensions
+      [@length, @width, @height]


This can be memoized too, if this object is correctly immutable.

kmcphillips · 2017-01-13T02:19:52Z

lib/active_shipping/package.rb

-      @oversized
+    def volume
+      @volume ||= if cylinder?
+        Math::PI * width.scale(0.5).value * height.scale(0.5).value * length.value


👌 dat-scale

But, you're switching to value here. And that makes the incorrect assumption that unit is the same on all of these.

Two things:

First, would the measured gem value from a function to coerce a list of values into the same unit? For example, this would be written as:

Measured.coerce_unit(width.scale(0.5), height.scale(0.5), length).reduce(&:*)

We could also add a function to do the inner product of the values until we support multiplying these together.

Second, I would recommend parentheses around the three terms on the right, because the first multiply will coerce things into a float and then you'll lose some precision gained from :value being Rational or BigDecimal. The other option would be to create a Rational or BigDecimal PI constant in this class and use that instead (recommended). The nice thing about that is that it will reduce the number of return value types.

kmcphillips · 2017-01-13T02:22:09Z

lib/active_shipping/package.rb

    end

    def cylinder?
-      @cylinder
+      @cylinder ||= @options[:cylinder].present?


This is then saying cylinder: "false" means that it is a cylinder.

Not necessarily wrong, but as long as we're ok with that. And document it. And are consistent.

Don't feel strong about this but I think that's an odd behaviour if someone is verbose about their options. I don't like using options hash where we largely leverage key/value but in some cases rely on the solely the key as a flag.

I'd recommend we should promote something other usage like:

options = { ... shape: :cylinder }

Then this becomes:

def cylinder? @cylinder ||= @options[:shape] == :cylinder end

Maybe shape isn't the best word, maybe package_type or type or mail_piece_shape. It also means we avoid where causes like

options: { cylinder: 'false', envelope: 'false', }

Also are we better to talk about box and tube which are pretty common packing types versus cylinder?

Great insight. I'm okay with shape, a Package generally has a shape - box, tube, or envelope. I won't enforce the enumeration of the shape, but I'll keep the boolean methods for common shapes e.g. box/tube/envelope.

kmcphillips · 2017-01-13T02:23:25Z

test/unit/package_test.rb

+    @weight = Measured::Weight(500, :grams)
+    @length = Measured::Length(30, :cm)
+    @width = Measured::Length(25, :cm)
+    @height = Measured::Length(15, :cm)


Switch one of these to another unit. That would have failed the .value bug I pointed out above.

kmcphillips · 2017-01-13T02:24:05Z

test/unit/package_test.rb

+    refute_predicate package, :tube?
+    refute_predicate package, :oversized?
+    refute_predicate package, :unpackaged?
+    refute_predicate package, :gift?


Thanks for changing to _predicate

kmcphillips · 2017-01-13T02:29:24Z

test/unit/package_test.rb

+  # This should return a Measured::Volume, but that doesn't exist yet.
+  # So we'll return a raw value like we have in the past until that is a thing.
+  test "#volume returns the volume for cylindrical package" do
+    skip "Requires Measured 2.0"


if Gem::Version.new(Measured::VERSION) < Gem::Version.new("2.0.0.pre1") skip "Requires Measured 2.0" end

thegedge · 2017-01-13T15:11:38Z

lib/active_shipping/package.rb

-      @oversized
+    def volume
+      @volume ||= if cylinder?
+        Math::PI * width.scale(0.5).value * height.scale(0.5).value * length.value


Two things:

First, would the measured gem value from a function to coerce a list of values into the same unit? For example, this would be written as:

Measured.coerce_unit(width.scale(0.5), height.scale(0.5), length).reduce(&:*)

We could also add a function to do the inner product of the values until we support multiplying these together.

Second, I would recommend parentheses around the three terms on the right, because the first multiply will coerce things into a float and then you'll lose some precision gained from :value being Rational or BigDecimal. The other option would be to create a Rational or BigDecimal PI constant in this class and use that instead (recommended). The nice thing about that is that it will reduce the number of return value types.

thegedge · 2017-01-13T15:17:49Z

lib/active_shipping/package.rb


-      @dimensions = [dimensions].flatten.reject(&:nil?)
+    def initialize(weight:, length:, width:, height: nil, options: {})
+      @options = @@default_options.merge(options.symbolize_keys)


If @@default_options can be changed by a client of this library then I'd recommend @@default_options.merge(options).symbolize_keys to guarantee all keys are symbols.

thegedge · 2017-01-13T15:22:38Z

lib/active_shipping/package.rb

-        process_dimensions
-      end
+      @value = self.class.cents_from(options[:value])
+      @currency = options[:currency] || options[:value].try(:currency)


What if I pass in the following Hash for options:

{ value: MoneyWithCurrency.new(10, currency: :usd), currency: :cad }

Then I'd question why the input is mismatched internally.

It's a matter of prioritizing an explicit value given for the options hash, vs. prioritizing the currency defined with the value object that might respond to currency.

I can see the value in either, but I favoured the input that was explicitly given.

I think I would actually trust the currency attached to value more than a given currency, but in the example I gave there's an inconsistency, so I think it would be fair to raise ArgumentError.

mdking

Fantastic start, just a few comments.

mdking · 2017-01-13T15:55:27Z

lib/active_shipping/package.rb

    end

    def cylinder?
-      @cylinder
+      @cylinder ||= @options[:cylinder].present?


Don't feel strong about this but I think that's an odd behaviour if someone is verbose about their options. I don't like using options hash where we largely leverage key/value but in some cases rely on the solely the key as a flag.

I'd recommend we should promote something other usage like:

options = { ... shape: :cylinder }

Then this becomes:

def cylinder? @cylinder ||= @options[:shape] == :cylinder end

Maybe shape isn't the best word, maybe package_type or type or mail_piece_shape. It also means we avoid where causes like

options: { cylinder: 'false', envelope: 'false', }

Also are we better to talk about box and tube which are pretty common packing types versus cylinder?

mdking · 2017-01-13T15:58:40Z

lib/active_shipping/package.rb

    end
    alias_method :tube?, :cylinder?

    def gift?
-      @gift
+      @gift ||= @options[:gift].present?


Again I think this could be expressed as a type of reason for export. There are a few like sale, gift, etc.. see image below. Therefore this could become

def gift? @gift ||= @options[:export_reason] == :gift end

~~export_reason is a little strangely worded to me, mostly for the export included. Maybe mail_reason? ship_reason?~~

Actually, since this is a customs concern, export is perfectly natural. I'll go with export_reason unless somebody has arguments for otherwise.

mdking · 2017-01-13T16:05:21Z

lib/active_shipping/package.rb

-      @centimetres ||= @dimensions.map { |m| m.convert_to(:cm).value }
-      measurement.nil? ? @centimetres : measure(measurement, @centimetres)
+    def oversized?
+      @oversized ||= @options[:oversized].present?


Again I think we should look at what this is describing. It's a carrier's view of physical rating characteristics of a package.

e.g. for Canada Post it might be

options = { rating_characteristics: [:oversized] }

Where as for USPS it might be

options = { rating_characteristics: [:oversized, :cubic]

Thoughts?

That's a bit wordy, but best describes it since characteristics is too vague.

I'm okay with rating_classifications as well.

jonathankwok

Great comments, friends. Will address and re-ping.

jonathankwok · 2017-01-13T18:45:39Z

lib/active_shipping/package.rb

-        process_dimensions
-      end
+      @value = self.class.cents_from(options[:value])
+      @currency = options[:currency] || options[:value].try(:currency)


Then I'd question why the input is mismatched internally.

It's a matter of prioritizing an explicit value given for the options hash, vs. prioritizing the currency defined with the value object that might respond to currency.

I can see the value in either, but I favoured the input that was explicitly given.

jonathankwok · 2017-01-13T19:00:58Z

lib/active_shipping/package.rb

    end

    def cylinder?
-      @cylinder
+      @cylinder ||= @options[:cylinder].present?


Great insight. I'm okay with shape, a Package generally has a shape - box, tube, or envelope. I won't enforce the enumeration of the shape, but I'll keep the boolean methods for common shapes e.g. box/tube/envelope.

jonathankwok · 2017-01-13T19:02:49Z

lib/active_shipping/package.rb

    end
    alias_method :tube?, :cylinder?

    def gift?
-      @gift
+      @gift ||= @options[:gift].present?


~~export_reason is a little strangely worded to me, mostly for the export included. Maybe mail_reason? ship_reason?~~

Actually, since this is a customs concern, export is perfectly natural. I'll go with export_reason unless somebody has arguments for otherwise.

jonathankwok · 2017-01-13T19:05:13Z

lib/active_shipping/package.rb

-      @centimetres ||= @dimensions.map { |m| m.convert_to(:cm).value }
-      measurement.nil? ? @centimetres : measure(measurement, @centimetres)
+    def oversized?
+      @oversized ||= @options[:oversized].present?


That's a bit wordy, but best describes it since characteristics is too vague.

I'm okay with rating_classifications as well.

jonathankwok · 2017-01-13T21:39:08Z

Had a great conversation with @mdking about rating concerns vs generic package concerns. TLDR

rating_classification shouldn't be an attribute on a Package, that's a rating concern. It should be on a RateEstimate
export_reason is the same, for the reasons above. A Package independently has no export reason. It should be attached to either a RateEstimate or the Shipment that holds the package.
shape can be kept on a package. It still describes a Package independent of the rate/shipment.

Will make these changes and iterate, but as always, everything is fluid and additional feedback is super welcome.

- remove shipment concerns from package (i.e. gift/oversized/unpackaged) - default shape to box and create tube/envelope predicate methods - some code style fixes/improvements

kevin-jj · 2017-05-31T02:49:25Z

What about this PR in the version 2.0? @jonathankwok @anthonyn60

jonathankwok · 2017-07-10T22:08:01Z

I don't quite have the time to work on this, so if somebody wants to see this merged they can pick up where I left off somewhere in a new PR.

Change Package interface for ActiveShipping 2.0

1ef5c00

kmcphillips reviewed Jan 13, 2017

View reviewed changes

thegedge reviewed Jan 13, 2017

View reviewed changes

mdking reviewed Jan 13, 2017

View reviewed changes

jonathankwok commented Jan 13, 2017

View reviewed changes

This was referenced Jan 16, 2017

Move rate-related package concerns to a RateEstimate/Shipment model #470

Open

Incorrect Rates #471

Open

Code review changes:

7dcc362

- remove shipment concerns from package (i.e. gift/oversized/unpackaged) - default shape to box and create tube/envelope predicate methods - some code style fixes/improvements

garethson force-pushed the master branch 3 times, most recently from 3847e9c to 448a674 Compare June 8, 2017 17:08

jonathankwok closed this Jul 10, 2017

jonathankwok deleted the package-2.0 branch July 10, 2017 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Package interface for ActiveShipping 2.0 #469

Change Package interface for ActiveShipping 2.0 #469

jonathankwok commented Jan 12, 2017

kmcphillips left a comment

kmcphillips Jan 13, 2017

kmcphillips Jan 13, 2017

kmcphillips Jan 13, 2017

kmcphillips Jan 13, 2017

thegedge Jan 13, 2017

kmcphillips Jan 13, 2017

kmcphillips Jan 13, 2017

mdking Jan 13, 2017

jonathankwok Jan 13, 2017

kmcphillips Jan 13, 2017

kmcphillips Jan 13, 2017

kmcphillips Jan 13, 2017

thegedge Jan 13, 2017

thegedge Jan 13, 2017

thegedge Jan 13, 2017

jonathankwok Jan 13, 2017

thegedge Jan 13, 2017

mdking left a comment

mdking Jan 13, 2017

mdking Jan 13, 2017

jonathankwok Jan 13, 2017

mdking Jan 13, 2017

jonathankwok Jan 13, 2017

jonathankwok left a comment

jonathankwok Jan 13, 2017

jonathankwok Jan 13, 2017

jonathankwok Jan 13, 2017

jonathankwok Jan 13, 2017

jonathankwok commented Jan 13, 2017

kevin-jj commented May 31, 2017

jonathankwok commented Jul 10, 2017

Change Package interface for ActiveShipping 2.0 #469

Change Package interface for ActiveShipping 2.0 #469

Conversation

jonathankwok commented Jan 12, 2017

Preface

Changes

This is incomplete!

kmcphillips 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

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

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

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

mdking 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathankwok 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

Choose a reason for hiding this comment

jonathankwok commented Jan 13, 2017

kevin-jj commented May 31, 2017

jonathankwok commented Jul 10, 2017