Merge lp:~savoirfairelinux-openerp/lp-community-utils/nag_refactor into lp:lp-community-utils

[Sandy Carter (http://www.savoirfairelinux.com)]

Moved functions used in openerp-nag to lp.py so they may be used by other scripts.

As of now, openerp-nag cannot be imported by other scripts due to the dash in its name and the fact it does not end in .py

No code was changed with the exception of the addition of the launchpad_login function which replicates old behaviour.

I have not done any of the cleanup of my old proposal as they can be done later.

Please review this with high priority as any future changed to openerp-nag means I have to rebase this commit.

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

Sorry I didn't saw your proposal before (even if I use openerp-nag...) whereas it deserves some attention.
Thanks for your work so far.

Seems good to me even if some changes seems not really necessary to me, surely you had some reasons.

What is the rationale to change the Nag namedtuple to a class? A reason could be that you added methods, but not. On that topic, couldn't we move the big print of the end to a method on Nag() (it seems that it could even be the __str__).

Couldn't already the module be used as a lib? (as there is if __name__ == "__main__":) If the filename is an issue (it is, i we want to import it), maybe we can consider to rename it? Not that I strongly disagree with the split but it seem a bit overkill to me here. But maybe do you have great plans to extend this tool and so it makes more sense ;-)

I strongly disagree to remove the --authenticated option: the Launchpad API is limited with anonymous users and it is used now for the votes (the votes are only visible by authenticated users). BTW you may want to rebase on the master since votes have been integrated meanwhile.

What was the bug with the age?

review: Needs Fixing (code review)

[Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter)]

@gbaconnier-c2c
Good point about adding functions to Nag, the reason for turning it into a class is design and optimization.
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.

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.

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

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

[Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter)]

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

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

On 03/10/2014 02:12 PM, Sandy Carter (http://www.savoirfairelinux.com)
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.
>

Enlightening, thanks for the explanation.

[Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter)]

@Guewen
> Couldn't already the module be used as a lib? (as there is if __name__ ==
> "__main__":) If the filename is an issue (it is, i we want to import it),
> maybe we can consider to rename it?

The problem right now is that you can't import from it due to its filename (dash in filename and no .py)

[Stefan Rijnhart (Opener) (stefan-opener)]

I would prefer to keep it in one file and rename to openerp_nag.py then. Would that work for you?

review: Needs Information

[Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter)]

@Stefan, yes that would work, we could even be backwards compatible and have a symlink
openerp_nag -> openerp_nag.py

[Stefan Rijnhart (Opener) (stefan-opener)]

Great, setting to Needs Fixing pending such a refactoring.

review: Needs Fixing

Unmerged revisions

29. By Sandy Carter (http://www.savoirfairelinux.com) on 2014-04-06

Moved common code out of openerp-nag into lp.py so it can be used by other scripts

=== added file 'lp.py'
--- lp.py	1970-01-01 00:00:00 +0000
+++ lp.py	2014-04-06 16:40:59 +0000
@@ -0,0 +1,252 @@
+# Copyright 2012 Canonical Ltd.
+# Written by:
+#   Zygmunt Krynicki <[e-mail]>
+# Hacked for the OpenERP Community Reviewers version by:
+#   Guewen Baconnier <[e-mail]>
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3,
+# as published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+from __future__ import unicode_literals, print_function
+
+import collections
+import datetime
+import logging
+
+try:
+    from progressbar import ProgressBar, Bar, Percentage, ETA
+except ImportError:
+    ProgressBar = None
+
+from launchpadlib.launchpad import Launchpad
+
+
+# Nag:
+# Indication that we want to nag the 'person'...
+# ...with the specified 'action'
+# ...about a particular 'subject'
+Nag = collections.namedtuple(
+    "Nag", "person action subject sort_class sort_priority sort_age project_name votes")
+
+SORT_CLASS_BUG, SORT_CLASS_MERGE = range(2)
+
+
+def show_lp_object(obj):
+    print(obj)
+    print("attributes:", obj.lp_attributes)
+    for attr in obj.lp_attributes:
+        print("attribute[%s]: %s" % (attr, getattr(obj, attr)))
+    print("entries:", obj.lp_entries)
+    for entry in obj.lp_entries:
+        print("entry[%s]: %s" % (entry, getattr(obj, entry)))
+    print("collections:", obj.lp_collections)
+    print("operations:", obj.lp_operations)
+    print("-" * 80)
+
+
+class DefaultPolicy:
+
+    mps_need_commit_message = False
+
+    max_review_age = 0
+
+    max_bug_new_age = 7
+
+    # minimal number of reviewers before merging
+    min_approve = 2
+
+    # can't be merged before n days
+    days_before_merge = 5
+
+    # can be merged before days_before_merge if at least n approvals
+    approvals_to_bypass = 3
+
+
+class UTC(datetime.tzinfo):
+
+    def dst(self, foo):
+        return datetime.timedelta(0, 0)
+
+    def utcoffset(self, foo):
+        return datetime.timedelta(0, 0)
+
+
+UTC = UTC()
+
+
+class Votes(object):
+    """ Holds votes of a proposal """
+
+    __states__ = {
+        'Approve': 'approve',
+        'Needs Fixing': 'needs_fixing',
+        'Needs Information': 'needs_information',
+        'Abstain': 'abstain',
+        'Disapprove': 'disapprove',
+        'Resubmit': 'resubmit',
+        'Pending': 'pending',
+    }
+
+    __signs__ = {
+        'approve': '+',
+        'needs_fixing': '!',
+        'needs_information': '?',
+        'abstain': '~',
+        'disapprove': '-',
+        'resubmit': 'R',
+        'pending': '*',
+    }
+
+    __ignore_for_approval__ = ['abstain', 'pending']
+
+    def __init__(self, votes):
+        self._votes = collections.Counter(
+            self.__states__[vote.comment.vote if vote.comment else 'Pending']
+            for vote in votes
+        )  # no comment is a pending review, usually the team
+
+    def __getattr__(self, name):
+        if name in self._votes:
+            return self._votes[name]
+        elif name in self.__states__.values():
+            return 0
+        else:
+            raise AttributeError
+
+    def __str__(self):
+        signs = ['%d%s' % (count, self.__signs__[state]) for
+                 state, count in self._votes.iteritems()]
+        return ', '.join(signs)
+
+    def total(self, for_approval=False):
+        if for_approval:
+            return sum(count for state, count in self._votes.iteritems()
+                       if state not in self.__ignore_for_approval__)
+        return sum(self._votes.values())
+
+    @classmethod
+    def legend(cls):
+        return dict((state, cls.__signs__[code]) for
+                    state, code in cls.__states__.iteritems())
+
+
+def gen_project_nags(lp, policy, project_name):
+    # TODO: Detect project groups and redirect the nag to all projects
+    # underneath that project
+    # Access the project that we care about
+    logging.debug("Accessing project %r", project_name)
+    project = lp.projects[project_name]
+    # Re-yield all the merge proposal nags
+    # NOTE: change to yield from in py3k3
+    for nag in gen_merge_proposal_nags(lp, policy, project):
+        yield nag
+
+
+def gen_merge_proposal_nags(lp, policy, project):
+    # XXX: cannnot use utcnow as launchpad returns tz-aware objects here
+    now = datetime.datetime.now(tz=UTC)
+    logging.debug("Looking for merge proposals in project %r", project)
+    # NOTE: Workaround for project.getMergeProposals() crashing on timeouts
+    try:
+        merge_proposals = project.getMergeProposals(status='Needs review')
+    except AttributeError:
+        logging.warning("The project %s has no code to look at", project)
+        return
+    for proposal in merge_proposals:
+        logging.debug("Looking at merge proposal %r", proposal)
+        # Skip everything that is still not requested for review
+        if proposal.date_review_requested is None:
+            continue
+        # Skip everything that is already merged
+        if proposal.date_merged is not None:
+            continue
+        votes = Votes(proposal.votes)
+
+        # Nag about missing commit message on merge requests
+        if policy.mps_need_commit_message and proposal.commit_message is None:
+            yield Nag(
+                person=proposal.registrant.display_name,
+                action="set a commit message on merge request",
+                subject=proposal.web_link,
+                sort_class=SORT_CLASS_MERGE,
+                sort_priority=None,  # TODO: get from max(linked bugs)
+                sort_age=(proposal.date_review_requested - now).days,
+                project_name=project.name,
+                votes=votes,
+            )
+
+        age = (now - proposal.date_review_requested).days
+
+        if (votes.approve == votes.total(for_approval=True) and
+                (votes.approve >= policy.approvals_to_bypass or
+                 votes.approve >= policy.min_approve and
+                 age >= policy.days_before_merge)):
+            yield Nag(
+                person='A committer',
+                action="consider to merge the proposal",
+                subject=proposal.web_link,
+                sort_class=SORT_CLASS_MERGE,
+                sort_priority=None,  # TODO: get from max(linked bugs)
+                sort_age=(proposal.date_review_requested - now).days,
+                project_name=project.name,
+                votes=votes,
+            )
+            continue
+
+        # Nag about aging merge requests
+        if age >= policy.max_review_age:
+            yield Nag(
+                person='Someone',
+                action="review the merge request",
+                subject=proposal.web_link,
+                sort_class=SORT_CLASS_MERGE,
+                sort_priority=None,  # TODO: get from max(linked bugs)
+                sort_age=(proposal.date_review_requested - now).days,
+                project_name=project.name,
+                votes=votes,
+            )
+
+
+def gen_bug_nags(lp, policy, project):
+    now = datetime.datetime.now(tz=UTC)
+    new_threshold = now - datetime.timedelta(days=policy.max_bug_new_age)
+    # Nag about bugs that are "New" for too long
+    logging.debug("Looking at 'new' bugs that are 8 days or older")
+    aging_new_bugs = project.searchTasks(status='New',
+                                         created_before=new_threshold)
+    for bug_task in aging_new_bugs:
+        yield Nag(
+            person='everyone',
+            action='triage the bug',
+            subject=bug_task.web_link,
+            sort_class=SORT_CLASS_BUG,
+            sort_priority=None,  # TODO: convert from importance name
+            sort_age=None,
+            votes=None)
+
+
+def parse_projects_file(filename):
+    """ Parse a file containing name of the projects
+    A line per project
+    """
+    with open(filename, mode='rU') as project_file:
+        content = project_file.read()
+
+    return [name.strip() for name in content.splitlines()]
+
+
+def launchpad_login(anonymous, consumer_name, service_root):
+    if anonymous:
+        return Launchpad.login_anonymously(consumer_name, service_root)
+    else:
+        return Launchpad.login_with(consumer_name, service_root)

=== modified file 'openerp-nag'
--- openerp-nag	2014-02-20 16:18:06 +0000
+++ openerp-nag	2014-04-06 16:40:59 +0000
@@ -42,234 +42,16 @@
 from __future__ import unicode_literals, print_function
 
 import argparse
-import collections
-import datetime
-import logging
 
 try:
     from progressbar import ProgressBar, Bar, Percentage, ETA
 except ImportError:
     ProgressBar = None
-from launchpadlib.launchpad import Launchpad
-
+from lp import *
 
 consumer_name = 'OpenERP Community Reviewers Nagging Scripts'
 
 
-# Nag:
-# Indication that we want to nag the 'person'...
-# ...with the specified 'action'
-# ...about a particular 'subject'
-Nag = collections.namedtuple(
-    "Nag", "person action subject sort_class sort_priority sort_age project_name votes")
-
-SORT_CLASS_BUG, SORT_CLASS_MERGE = range(2)
-
-
-def show_lp_object(obj):
-    print(obj)
-    print("attributes:", obj.lp_attributes)
-    for attr in obj.lp_attributes:
-        print("attribute[%s]: %s" % (attr, getattr(obj, attr)))
-    print("entries:", obj.lp_entries)
-    for entry in obj.lp_entries:
-        print("entry[%s]: %s" % (entry, getattr(obj, entry)))
-    print("collections:", obj.lp_collections)
-    print("operations:", obj.lp_operations)
-    print("-" * 80)
-
-
-class DefaultPolicy:
-
-    mps_need_commit_message = False
-
-    max_review_age = 0
-
-    max_bug_new_age = 7
-
-    # minimal number of reviewers before merging
-    min_approve = 2
-
-    # can't be merged before n days
-    days_before_merge = 5
-
-    # can be merged before days_before_merge if at least n approvals
-    approvals_to_bypass = 3
-
-
-class UTC(datetime.tzinfo):
-
-    def dst(self, foo):
-        return datetime.timedelta(0, 0)
-
-    def utcoffset(self, foo):
-        return datetime.timedelta(0, 0)
-
-
-UTC = UTC()
-
-
-class Votes(object):
-    """ Holds votes of a proposal """
-
-    __states__ = {
-        'Approve': 'approve',
-        'Needs Fixing': 'needs_fixing',
-        'Needs Information': 'needs_information',
-        'Abstain': 'abstain',
-        'Disapprove': 'disapprove',
-        'Resubmit': 'resubmit',
-        'Pending': 'pending',
-    }
-
-    __signs__ = {
-        'approve': '+',
-        'needs_fixing': '!',
-        'needs_information': '?',
-        'abstain': '~',
-        'disapprove': '-',
-        'resubmit': 'R',
-        'pending': '*',
-    }
-
-    __ignore_for_approval__ = ['abstain', 'pending']
-
-    def __init__(self, votes):
-        self._votes = collections.Counter(
-            self.__states__[vote.comment.vote if vote.comment else 'Pending']
-            for vote in votes
-        )  # no comment is a pending review, usually the team
-
-    def __getattr__(self, name):
-        if name in self._votes:
-            return self._votes[name]
-        elif name in self.__states__.values():
-            return 0
-        else:
-            raise AttributeError
-
-    def __str__(self):
-        signs = ['%d%s' % (count, self.__signs__[state]) for
-                 state, count in self._votes.iteritems()]
-        return ', '.join(signs)
-
-    def total(self, for_approval=False):
-        if for_approval:
-            return sum(count for state, count in self._votes.iteritems()
-                       if state not in self.__ignore_for_approval__)
-        return sum(self._votes.values())
-
-    @classmethod
-    def legend(cls):
-        return dict((state, cls.__signs__[code]) for
-                    state, code in cls.__states__.iteritems())
-
-
-def gen_project_nags(lp, policy, project_name):
-    # TODO: Detect project groups and redirect the nag to all projects
-    # underneath that project
-    # Access the project that we care about
-    logging.debug("Accessing project %r", project_name)
-    project = lp.projects[project_name]
-    # Re-yield all the merge proposal nags
-    # NOTE: change to yield from in py3k3
-    for nag in gen_merge_proposal_nags(lp, policy, project):
-        yield nag
-
-
-def gen_merge_proposal_nags(lp, policy, project):
-    # XXX: cannnot use utcnow as launchpad returns tz-aware objects here
-    now = datetime.datetime.now(tz=UTC)
-    logging.debug("Looking for merge proposals in project %r", project)
-    # NOTE: Workaround for project.getMergeProposals() crashing on timeouts
-    try:
-        merge_proposals = project.getMergeProposals(status='Needs review')
-    except AttributeError:
-        logging.warning("The project %s has no code to look at", project)
-        return
-    for proposal in merge_proposals:
-        logging.debug("Looking at merge proposal %r", proposal)
-        # Skip everything that is still not requested for review
-        if proposal.date_review_requested is None:
-            continue
-        # Skip everything that is already merged
-        if proposal.date_merged is not None:
-            continue
-        votes = Votes(proposal.votes)
-
-        # Nag about missing commit message on merge requests
-        if policy.mps_need_commit_message and proposal.commit_message is None:
-            yield Nag(
-                person=proposal.registrant.display_name,
-                action="set a commit message on merge request",
-                subject=proposal.web_link,
-                sort_class=SORT_CLASS_MERGE,
-                sort_priority=None,  # TODO: get from max(linked bugs)
-                sort_age=(proposal.date_review_requested - now).days,
-                project_name=project.name,
-                votes=votes,
-            )
-
-        age = (now - proposal.date_review_requested).days
-
-        if (votes.approve == votes.total(for_approval=True) and
-                (votes.approve >= policy.approvals_to_bypass or
-                 votes.approve >= policy.min_approve and
-                 age >= policy.days_before_merge)):
-            yield Nag(
-                person='A committer',
-                action="consider to merge the proposal",
-                subject=proposal.web_link,
-                sort_class=SORT_CLASS_MERGE,
-                sort_priority=None,  # TODO: get from max(linked bugs)
-                sort_age=(proposal.date_review_requested - now).days,
-                project_name=project.name,
-                votes=votes,
-            )
-            continue
-
-        # Nag about aging merge requests
-        if age >= policy.max_review_age:
-            yield Nag(
-                person='Someone',
-                action="review the merge request",
-                subject=proposal.web_link,
-                sort_class=SORT_CLASS_MERGE,
-                sort_priority=None,  # TODO: get from max(linked bugs)
-                sort_age=(proposal.date_review_requested - now).days,
-                project_name=project.name,
-                votes=votes,
-            )
-
-
-def gen_bug_nags(lp, policy, project):
-    now = datetime.datetime.now(tz=UTC)
-    new_threshold = now - datetime.timedelta(days=policy.max_bug_new_age)
-    # Nag about bugs that are "New" for too long
-    logging.debug("Looking at 'new' bugs that are 8 days or older")
-    aging_new_bugs = project.searchTasks(status='New',
-                                         created_before=new_threshold)
-    for bug_task in aging_new_bugs:
-        yield Nag(
-            person='everyone',
-            action='triage the bug',
-            subject=bug_task.web_link,
-            sort_class=SORT_CLASS_BUG,
-            sort_priority=None,  # TODO: convert from importance name
-            sort_age=None,
-            votes=None)
-
-
-def parse_projects_file(filename):
-    """ Parse a file containing name of the projects
-    A line per project
-    """
-    with open(filename, mode='rU') as project_file:
-        content = project_file.read()
-
-    return [name.strip() for name in content.splitlines()]
-
-
 def main():
     parser = argparse.ArgumentParser()
     group = parser.add_argument_group(title="debugging")
@@ -304,10 +86,7 @@
     if args.debug:
         logging.basicConfig(level=logging.DEBUG)
     # Access Lauchpad object as the current (system) user or anonymously
-    if args.anonymous:
-        lp = Launchpad.login_anonymously(consumer_name, args.service_root)
-    else:
-        lp = Launchpad.login_with(consumer_name, args.service_root)
+    lp = launchpad_login(args.anonymous, consumer_name, args.service_root)
 
     if not args.project and not args.projects_file:
         parser.print_usage()