cucumber/cucumber — Issue #202: Column mapping dangerously applied twice to the same cell!
Author: jbrains
Created: 2011-12-11 00:38:19+00:00
State: open

Wow. This was fun. I don't know how this ever worked. By coincidence, is everyone's column mapping proc idempotent?! That would surprise me.

Here's the minimal failing spec (why Money matters will be apparent soon):

gem 'money'
require 'money'
describe "Mapping a column" do
example "my conversion proc is called twice, and that sucks" do
@table = Cucumber::Ast::Table.new([["Receipt Amount"], ["CAD 100"]])
conversion_proc_for_receipt_amount = mock()
conversion_proc_for_receipt_amount.should_receive(:call).once.and_return(Money.new(10000, "CAD"))
@table.map_column!("Receipt Amount") { |text| conversion_proc_for_receipt_amount.call }
@table.hashes.should == [{"Receipt Amount" => Money.new(10000, "CAD")}]
end
end

This spec fails, because it calls my column mapping proc twice. For example, this causes Table to try to parse "CAD 100" into a Money, then parse that Money object again, which happens to do something weird and annoying. Clearly, that ain't right. Without some weird hack, like checking that `text` is really text, I don't know how to make that conversion proc idempotent. That would seem like an unreasonable requirement.

The two calls appear to be in `convert_columns!` and `to_hash`. Without digging deeper, it seems to me that `to_hash` ought not to convert the columns directly, when it could use `convert_columns!`, and that method could memoize the result.

I don't have the energy to fix this now, and even so, I'd like someone there to confirm that it's indeed a mistake, and that you didn't intend for the conversion procs to need to be idempotent.

Thanks!