[trep-engine.scm] upgrade engine to improve calculated-cells handling

[christopherlam]

This branch offers an upgrade to trep-engine so that the calculator-fn handles both split and transaction-row?. calculated-cells is now a versioned record cells, each cell has 7 fields.

for use by #1617 by @dawansv -- instead of vector you'll need to make-cell.

Note income-gst-statement.scm stil offers the previous list of vectors, each vector has 6 fields, as a proof of backward compatibility.

EDIT: instead of records, it may be safer to use an assoc-list which is easier to upgrade eg.

     (if (column-uses? 'amount-single)
              (list (list (cons 'header (header-commodity (G_ "Amount")))
                          (cons 'calc-fn converted-amount)
                          (cons 'reverse-col? #t) 
                          (cons 'subtotal? #t)
                          (cons 'friendly-heading-fn (const ""))
                          (cons 'merge-dual-column #f)))
              '())

[dawansv]

This is very nice! Lots to like here.

I like the idea of using a version 2 structure and then using the conversion procedure. Very clean.

I actually quite like the use of record because the field names used throughout the code when values are retrieved make the code so much more readable than the use of vector-ref. The assoc-list would work as well since it would show the key name.

The use of records has the added benefit of leaving the current column definition almost unchanged except for the vector -> make-cell conversion. So if people want to upgrade custom reports to v2 to get the extra parameter, it is quite easy with only that one change and the additional tr? parameter in the calculator functions.

Also although it's nice to see the key/value pair in the column definition with the assoc-list, it's a lot of extra code repeating these on every column, and I guess another potential source for errors? You mention it might be safer so that could be a +, although I don't quite have the knowledge to know why.

All in all this is a solid upgrade I think.

[christopherlam]

Also although it's nice to see the key/value pair in the column definition with the assoc-list, it's a lot of extra code repeating these on every column, and I guess another potential source for errors? You mention it might be safer so that could be a +, although I don't quite have the knowledge to know why.

Yes it's a source of errors, however (assq-ref alist 'inexistent-symbol) will return #f instead of crashing. It's also safer and easier to further upgrade to add another metadata, simply by adding another pair. It will not require versioning at all; it becomes a features-based upgrade.

[christopherlam]

Modified. Remove arbitrary versioning. Maintain legacy vector to alist upgrade.

[dawansv]

That's pretty nice as well. I like your invalid-cell? function doing a good job at replacing what the record was doing in terms of making sure the required "fields" are propertly named. Having the keys in the column definitions definitely makes the code more readable. And I like the use of the cut macro to retrieve the values to avoid having to revert back to the longer lambda form that the use of record had gotten rid of.