> 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 : [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
