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

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) 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

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

> 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