Review: Needs Information surface scan, no test I understand it would sometimes be useful to disable the default aggregation mechanism for some columns, however I have some objections to this MP:
1. This is an API change targeted at 7.0, and one for a very wishlist behavior. I think this really belongs to trunk. For 7.0 there are some possible workarounds: - use a `group_operator` that gives a more acceptable behavior, such as `min`, `count` or perhaps `NULL*count` that would always render as 0.0. - if that's still not good enough, write a small helper function that returns a special read_group() function that drops the relevant column(s) from the result given by the super() call. Assign this special function as the read_group() method in models that need it. 2. If we intend to improve the API in trunk to support a finer-grained control on aggregation, we should use this opportunity to improve the design further and really delegate the aggregation decision to the fields themselves, instead of mostly hardwiring it in read_group(). This should only be a small variation to your patch, so if it seems acceptable to you, you could update your branch and resubmit it for trunk. Apart from the above, does your patch actually work in its current state? (I did not test) It seems read_group() is accessing an 'allow_aggregation' item that fields_get() never returns, which should cause KeyErrors for any column except id/sequence? Thanks! -- https://code.launchpad.net/~credativ/ocb-server/7.0-lp1166886/+merge/182853 Your team credativ is subscribed to branch lp:~credativ/ocb-server/7.0-lp1166886. -- Mailing list: https://launchpad.net/~credativ Post to : [email protected] Unsubscribe : https://launchpad.net/~credativ More help : https://help.launchpad.net/ListHelp
