openstack team mailing list archive

[Rick Harris <rick.harris@xxxxxxxxxxxxx>]

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