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