[rfc] Trigger Plugin

[Robert Collins]

Just a few thoughts

On Fri, 2006-01-13 at 15:33 -0500, Nathaniel McCallum wrote:
> """Provides trigger support"""

you need to put some sort of copy right statement here for people to be
able to use this. 

> import os
> 
> import bzrlib
> import bzrlib.lock
> from bzrlib.commands import Option, Command, display_command

Alphabetical order here (PEP8-ness)

> class TriggerCache(object):
>     
>     filename = os.path.join(bzrlib.config.config_dir(), "trigger")
>     delimiter = "|"

This is a purely aesthetic thing - but I find that class scope variables
(filename and delimiter here) that are really instance variables
hideously confusing and brittle. As you have an __init__ you dont save
any space at all by having them in class scope.
    
>     def __init__(self):
>         self.records = {}
>         
>         # Make sure we have the parent directory
>         dirname = os.path.dirname(self.filename)
>         if os.path.exists(dirname) and not os.path.isdir(dirname):
>             os.remove(dirname)
>         if not os.path.exists(dirname):
>             os.mkdir(dirname)
>         
>         # Lock the file
>         self._lock = bzrlib.lock.WriteLock(os.path.join(dirname,
> ".trigger-lock"))
>         
>         # Read the file
>         self.read()

For portability, have a look in bzrlib.osutils We have wrappers for
os.path.join that do the right thing for the bzr coding standards, as
well as trivial forwarders for most routines that we anticipate changing
in the future.


>     def __del__(self):
>         self._lock.unlock()

Using __del__ is notoriously unreliable - if someone uses this trigger
from anything other than the command line - expect hung locks. I
encourage you to provide an explicit 'finished' or some such that will
release the lock. You seem to be following roughly the RAII pattern that
is prominent in C++ - unfortunately in GC'd languages, and python in
particular, that becomes a bit of an anti-pattern.


>     def read(self):
>         """Read the file from disk"""
>         
>         # Try to read the file, just return on failure
>         try:
>             for line in open(self.filename):
>                 base, command, revno =
> line.strip().split(self.delimiter)
>                 revno = int(revno)
>                 
>                 key = (base, command)
>                 if not self.records.has_key(key) or self.records[key]
> < revno:

This could be written more pithily (and I think more clearly) as
                  if self.records.get(key, -1) < revno:

>                     self.records[key] = revno
>         except IOError:
>             pass
>         
>     def write(self):
>         """Write out the file to disk."""
> 
>         output = open(self.filename, "w")

The leading vertical white space here is quite confusing - PEP8 is:

    def write(self):
        """Self contained sentence."""
        code starts here.

    def next_method_here(self):
...

>         for (base, command), revno in self.records.items():
>             output.write(self.delimiter.join((base, command,
> str(revno))) + os.linesep)

You are missing a close on output. Its probable that output will be
closed, but not guaranteed. On windows that can mean the file cannot be
touched by others, on unix its a fd leak. 

            
>     def get_last_trigger(self, branch_base, command):
>         """Return the revno for the last time the command was run on
> the branch"""
>         
>         if self.records.has_key((branch_base, command)):
>             return self.records[(branch_base, command)]
>         else:
>             return 0

I like the explicit function for this. A smaller version though:
          return self.records.get((branch_base, command), 0)

        
>     def set_last_trigger(self, branch_base, command, revno):
>         """Set the last revno for the command on this branch"""
>         
>         self.records[(str(branch_base), str(command))] = int(revno)
>         
>     def run_command(self, branch_base, command, revno=None):
>         """Run a command on branch, optionally on the specified
> revno"""
>         
>         if revno == None:
>             revno = self.get_last_trigger()
>         
>         new_command = command.replace("%revno%", str(revno))
>         new_command = new_command.replace("%branch%", branch_base)
>         if os.system(new_command) != 0:
>             print "Command '%s' failed for revision #%d on branch '%
> s'!" % (new_command, revno, branch_base)
>             raise SystemError  # What should this exception be?
>         self.set_last_trigger(branch_base, command, revno)
>         self.write()

the subprocess module is a replacement for os.system - its more capable
and reliable. You might like to look at using it. For the exception just
define one:
class FailedCommand(Exception):
    """Failed to run a command."""

see the bzrlib.errors module for 'BzrNewError' for a nice way of
providing meaningfull error messages with exceptions.

> class cmd_trigger(Command):
>     """Performs commands once per revision
>     
>     Within your command, trigger will substitute:
>         '%branch%' -- for the absolute path to the branch
>         '%revno%'  -- for the revision the command is being called for
>     """
>     
>     takes_args = ['command*']
>     takes_options = ['root', Option('latest', help="Perform command on
> only the latest revision")]
>         
>     @display_command
>     def run(self, command_list, root=None, latest=False):
>         
>         # Open branch
>         if root == None:
>             root = "."
>         branch = bzrlib.branch.Branch.open_containing(root)[0]
> 
>         # Assemble command
>         command = " ".join(command_list)
>         
>         # Get last revno triggered
>         tc = TriggerCache()
>         last_revno = tc.get_last_trigger(branch.base, command)
>         
>         # Get branch's current revno
>         curr_revno = branch.revno()
>         
>         # If we have already run a command on this revno, exit
>         if curr_revno <= last_revno:
>             return
>         
>         # If we are going to do the command on all revisions, loop
> through them
>         if not latest:
>             for i in range(last_revno + 1, curr_revno):
>                 tc.run_command(branch.base, command, i)

If I saw a patch to this text to a new version, I'd be completely lost.
I'd be thinking 'whats tc' ?. Short variable names - eek. (i is really
non optimal too, but I (unfortunately ;)) cant argue that its unnclear
when it is only used on two lines.


>         # Do the command on the latest revision
>         tc.run_command(branch.base, command, curr_revno)
>         
>         
> bzrlib.commands.register_command(cmd_trigger) 

HTH,
Cheers,
Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060116/3ce3b647/attachment.pgp