[Code-review] review: Gadget alias branch
Guillaume Desmottes
guillaume.desmottes at collabora.co.uk
Wed Aug 20 04:12:00 EDT 2008
Le mardi 19 août 2008 à 18:13 +0100, Dafydd Harries a écrit :
> Ar 19/08/2008 am 12:30, ysgrifennodd Guillaume Desmottes:
> > Le mardi 19 août 2008 à 10:59 +0100, Dafydd Harries a écrit :
> > > Ar 18/08/2008 am 10:47, ysgrifennodd Guillaume Desmottes:
> > > > Le vendredi 15 août 2008 à 20:24 +0100, Dafydd Harries a écrit :
> > > > > This branch mostly looks great. Again, just one thing to change:
> > > > >
> > > > > >+ alias = elem.getAttribute('alias')
> > > > > >+
> > > > > >+ q.append(('search', jid, props, alias))
> > > > >
> > > > > We should insert a:
> > > > >
> > > > > if alias == '':
> > > > > alias = None
> > > > >
> > > > > If the alias attribute is '', then it will match every buddy, which I don't
> > > > > think is what we want.
> > > >
> > > > I already check that here:
> > > >
> > > > + if alias is not None:
> > > > + # have to check alias
> > > > + if buddy.alias is None or not alias_match(buddy.alias, alias):
> > > > + return False
> > >
> > > The attribute might be present, but with an empty value. We should check for
> > > that too, no?
> >
> > I added a check that should prevent model to contain '' as alias instead
> > of None.
> > http://monkey.collabora.co.uk/gadget_alias/patch-5.html
>
> That seems reasonable. I'm still not convinced that the right thing happens
> when alias='' in the query, though.
Oh indeed, you're totally right. I fixed that and merged the alias
branch to master.
Thanks for the review.
G.
More information about the Code-review
mailing list