Code review comment for lp:~savoirfairelinux-openerp/lp-community-utils/nag_refactor

Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

> > On 03/07/2014 07:45 PM, Sandy Carter (http://www.savoirfairelinux.com)
> > wrote:
> > @gbaconnier-c2c
> > Good point about adding functions to Nag, the reason for turning it
> > into a class is design and optimization.
> The rationale about design is very subjective and the one about
> optimization is dubious:
>
> http://hastebin.com/neyedexiju.py

Declaring a class puts members together in memory and reduces the rate of cache misses, you can see this when performing lookups, but mostly when doing operations.
Try this: http://hastebin.com/difomedika.py
That's close to 15% increase in speed for summing members.

>
> Anyway, if it needs methods, and I think the display should be one, then
> it makes sense.
>

I agree

> > It gives the option, like to said, to add functions but also, the
> > ability to inherit from it.
> >
> > I do have big plans with the lib, for example, automatic
> > diff-validating in the style of
> > (https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl),
> > specifically checking style, seeing if the diff introduces pep8
> > errors, removes them, testing the patches with jenkins, commenting on
> > the MPs, etc.
> > I have proposed something already:
> > https://code.launchpad.net/~savoirfairelinux-openerp/lp-community-
> utils/branch_pep8/+merge/205260
> >
> >
> > As for the --authenticated option, I think I took it away because it
> > wasn't referenced or used, I can put it back now, however I will have
> > no impact. This was a while ago, though, I could have just taken it
> > out accidentally.
>
> That's right at the time you made the proposal, the option seemed to be
> not useful.
>
> >
> > The problem with age was that it declared it negatively, then did * -1
> > again while sorting. I just took out the (-1 * -1 = 1) redundancy. It
> > also has a one off error where an MP from today is displayed as 1 day
> > old.
>
> Ok.

« Back to merge proposal