Croak if mixing up second argument to cost_for_plan

[kjetilk]

Since the bugs caused by mixing up the second argument, plan/model to cost_for_plan causes pretty obscure problems, I think we should make it croak if that happens. I looked into it (and therefore started feature-cost-ensure-planner), but then I became unsure how to do it... Since you can't just check does('Attean::API::CostPlanner') since they both do that. It could be implemented in that role, but would we check isa('Attean::QueryPlanner'), etc? That would then mean that all query planners would extend that instead of just composing everything...

[kasei]

Where do you want this check to happen? In the planner? Or the model? I can think of a few different options here:

• Check that the two arguments (one of which is $self) aren't the same type (or, that one and only one is a model object)
• Split Attean::API::CostPlanner so that the role that models consume when they can do some planning isn't identical to the one full query planners consume.

[kjetilk]

It seems the latter option isn't very attractive, since what the model and the planner does in this regard is very similar, and so should be a single role.

I think both the planner and the model should check, and it sounds like a reasonable check that they aren't the same time and only one is a model object.

[kasei]

They're very similar, yes. But models don't have to return a cost for all plans. They have the option of returning undef and letting the upstream planner handle it. The upstream planner is required to generate costs for all plans. That seems like a relevant difference, doesn't it?

For now, I can add the checks about the two objects not being the same.

[kjetilk]

Right, that's a good point! Any of them would have caught the previous error, so it works for me. Thanks!

[kjetilk]

Yup, this looks good. Would have caught the problem for sure!