On Tue, Aug 14, 2007 at 04:36:22PM -0500, Adrian Holovaty wrote: > After a cursory read-through the patch, my only major complaint is the > unnecessary cleverness of some of the code.
Alt example besides BackendOps/BackendCapabilities would be
appreciated (your phrasing implies there is more); those two classes
are frankly borderline frankenstein at the moment due to continual
mangling.
> The stuff in BackendOps,
> for example, could be rewritten in a much clearer way. (Granted, it
> would be longer, but that's an acceptable tradeoff, in my opinion.)
>
> For example, this is in the patch:
>
> class BackendOps(object):
> required_methods = ["%s_sql" % x for x in
> ('autoinc', 'date_extract', 'date_trunc', 'datetime_cast',
> 'deferrable', 'drop_foreignkey', 'fulltext_search', 'limit_offset',
> 'random_function', 'start_transaction', 'tablespace')]
> required_methods += ['last_insert_id', 'max_name_length',
> 'pk_default_value', 'sequence_name', 'get_trigger_name']
> required_methods = ["get_%s" % x for x in required_methods]
> required_methods = tuple(required_methods)
>
> And this is a much, much clearer way of expressing exactly the same thing:
>
> class BackendOps(object):
> required_methods = (
> 'get_autoinc_sql',
> 'get_date_extract_sql',
> 'get_date_trunc_sql',
> 'get_datetime_cast_sql',
> 'get_deferrable_sql',
> 'get_drop_foreignkey_sql',
> 'get_fulltext_search_sql',
> 'get_limit_offset_sql',
> 'get_random_function_sql',
> 'get_start_transaction_sql',
> 'get_tablespace_sql',
> 'get_last_insert_id',
> 'get_max_name_length',
> 'get_pk_default_value',
> 'get_sequence_name',
> 'get_get_trigger_name'
> )
>
> Not only is it clearer, but it saves us from having to do calculations
> at run time.
While that may skip a calculation, honestly surprised there is no
complaint about 2-10 lines below where it's shoving a curried func
into locals (half expected a knee jerk "yuck" on that one ;).
As indicated further up, those two classes will need another cleanup;
the currying/locals I'd advocate leaving in, but expanding
required_methods there is definitely doable- it's form right now is
just a left over from how I went about pinning down what ops were
'standard' for backends. Crap like that will be removed next time I
do a full walk of the changes (this round is basically a "this is the
api I'm proposing, and here is a working patch for the api").
So... any spots that don't read as just idiocy? Specific complaints
about the presented API, seperation, etc, is what I'd like to get if
possible- still have a lot of gutting to do internally anyways, so the
form of it ought to improve.
~harring
pgp9XynR1TjiN.pgp
Description: PGP signature
