> 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@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