Agree with both #1s as a start. And I wouldn't try to 'rip off the band-aid'
either.
I was bringing this up initially as I want to enforce *something* when
reviewing, but if we want to start 'fixing' things, we can start hitting small
sections of code to limit conflicts. The 'enforce_model' thing might be a bit
extreme right now. We may be able to get to a place where we feel we've got
everything... then dictify it all at that time and see what raises in tests.,
fixing those last things. Hm... although, I guess I really do like the ability
to turn something off if it's broken. :)
- Chris
On Dec 15, 2011, at 12:29 AM, Rick Harris wrote:
> ++ on moving to a consistent dict-style syntax.
>
> We could attempt to rip the Band-Aid off here by:
>
> 1. Rejecting any new patches which use the dot-notation
>
> 2. Trying to switch out to using dot-notation access all at once in one big
> 'fix-it-up' patch.
>
> I'm not convinced 2) is possible. Separating model objects from other kinds
> of objects will be time-consuming and error prone. Given the scope of this
> issue, I'm not sure this could be done without a real risk of prolonged,
> recurring breakage.
>
> Instead, I'd advocate a step-wise approach:
>
> 1. Reject any new patches which use the dot-notation
>
> 2. Add code which helps us identify and fix dot-notation usage throughout the
> codebase.
>
> The basic idea here is that we'd add a flag to fail loudly when a object is
> accessed using non-dict methods.
>
> This could be done by adding a decorator to the sqlalchemy/db/api functions
> such that instead of returning SQLAlchemy objects directly they instead
> return objects of type AttrAccessibleDict.
>
> An AttrAccessibleDict would be defined something like:
>
> class AttrAccessibleDict(dict)
> def __getattr__(self, attr):
> if FLAGS.enforce_model_dict_access:
> raise ModelObjectAccessError('must use dict-style access on model
> objects')
> else:
> return self[attr]
>
> def dictify_model(f):
> def wrapper(*args, **kwargs):
> rv = f(*args, **kwargs)
> attr_dict = convert_to_attr_accessible_dict(rv)
> return attr_dict
> return wrapper
>
> @dictify_model
> def instance_get(…..):
> pass
>
>
> 3. Make a concerted effort to fix the code over a period of a few weeks.
>
> Developers could set --enforce_model_dict_access to True and fix up as many
> cases as they can find. When they're tired of fixing cases or when more
> pressing things come along for the moment, they can temporarily set
> --enforce_model_dict_access back to False to that everything goes back to
> humming along smoothly.
>
> 4. Once the dot-notation has been excised, we can switch over to actually
> returning real Python dictionaries instead of AttrAccessibleDict objects.
>
>
> 5. Final step would be removing the cleanup code.
>
> Once everything is returning dictionaries, we would remove the decorator
> entirely and any other cleanup code we added along the way.
>
>
>
> Feels like this approach would be a bit roundabout, but each step feels safe,
> and it seem like it would get us to where we want to be in the course of a
> few weeks (perhaps a couple of months, worst case).
>
> Thoughts?
>
> -Rick
>
> On Dec 15, 2011, at 1:10 AM, Chris Behrens wrote:
>
>>
>> I've seen a number of patches lately that have code like this:
>>
>> instance = db.instance_get(...)
>> instance_uuid = instance.uuid
>>
>> instead of:
>>
>> instance_uuid = instance['uuid']
>>
>> There's a mix of usage throughout the code, and I know some people are just
>> matching the surrounding code. But, in a number of cases, I've asked for
>> these to be corrected to the latter, on assumption that the DB layer will be
>> returning dictionaries at some point vs the models. It also pushes the code
>> towards consistent usage. But I might be the only Nova Core member looking
>> at this and/or maybe my assumption is wrong.
>>
>> So, I ask here: Should Nova Core make an effort to reject patches with the
>> former format? Or did I miss any DB layer plans where the former format is
>> now preferred?
>>
>> - Chris
>>
>>
>>
>>
>>
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~openstack
>> Post to : [email protected]
>> Unsubscribe : https://launchpad.net/~openstack
>> More help : https://help.launchpad.net/ListHelp
>
>
> _______________________________________________
> Mailing list: https://launchpad.net/~openstack
> Post to : [email protected]
> Unsubscribe : https://launchpad.net/~openstack
> More help : https://help.launchpad.net/ListHelp
_______________________________________________
Mailing list: https://launchpad.net/~openstack
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openstack
More help : https://help.launchpad.net/ListHelp
