openstack team mailing list archive

[Rick Harris <rick.harris@xxxxxxxxxxxxx>]

> I was bringing this up initially as I want to enforce *something* when reviewing,

Yeah, I was just thinking that it could be a point of confusion if, for an extended period, we're in a state where new code has to use dict-style instance access, while nearby, older code still uses attr-accessing.

> 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

One issue with that is that many of our fakes will already be returning dicts, so the unit-tests may pass, while the actual code may still trigger an exception.

The functional tests will help here, but, I don't think there's a substitute for having --enforce_model=True and having everybody hammering at it for a few weeks.

If it can survive that with no problems, then I'll be somewhat confident….

On Dec 15, 2011, at 2:49 AM, Chris Behrens wrote:

> 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     : openstack@xxxxxxxxxxxxxxxxxxx
>>> Unsubscribe : https://launchpad.net/~openstack
>>> More help   : https://help.launchpad.net/ListHelp
>> 
>> 
>> _______________________________________________
>> Mailing list: https://launchpad.net/~openstack
>> Post to     : openstack@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~openstack
>> More help   : https://help.launchpad.net/ListHelp
>