-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jamu Kakar wrote:
> Jamu Kakar has proposed merging lp:~jkakar/bzr/custom-project-name into lp:bzr.
> 
> Requested reviews:
>     bzr-core (bzr-core)
> 
> The attached branch makes it possible to customize the program name
> used when help text is generated.  It introduces the following
> changes to make this possible:
> 
> - bzrlib.commands has a new ProgramInfo class with name and version
>   attributes.  They default to 'bzr' and bzrlib.__version__, but a
>   new one can be created with custom settings.  New get_program_info
>   and set_program_info functions can be used to manage the currently
>   registered program information.
> 
> - Substitution variables are used when generating help text.
>   program-name and program-version variables are available by
>   default, retrieved from registered program information.
> 
> - An individual Command can implement a get_help_variables method to
>   customize or provide variables above and beyond what's built-in.
> 
> - Help text that couldn't be fixed using substitution has been fixed
>   manually.
> 
> - The help text for cmd_alias has been updated to use the
>   program-name variable.  It's the only builtin command I've updated
>   because it's the only command I have immediate plans to reuse in
>   Commandant.  cmd_help and cmd_version might be reusable, but all
>   three of these commands will need some work to reuse cleanly, and
>   right now I only have plans to do the work required for cmd_alias.
> 
> 


There are a few stylistic things that we generally prefer. For example:


+def get_program_info():
+    """Get program information.
+
+    :return: A ProgramInfo instance.
+    """
+    global _program_info
+    if _program_info is None:
+        _program_info = ProgramInfo()
+    return _program_info
+
+def set_program_info(info):

^- two blank lines before 'set'


...

         cmds = list(commands.all_command_names())
         self.assertEqual(['called'], hook_calls)
         self.assertSubset(['foo', 'bar'], cmds)
+
+
+class TestProgramInfo(tests.TestCase):
+    """Tests for ProgramInfo, get_program_info and set_program_info."""
+
+    def test_program_info(self):
+        """
+        A ProgramInfo has a name and a version, which can be passed as
+        parameters during instantiation.
+        """
+        info = commands.ProgramInfo('name', 'version')
+        self.assertEquals(info.name, 'name')
+        self.assertEquals(info.version, 'version')

^- Generally we use "assertEqual()" and put the expected form on the left.

self.assertEqual('name', info.name)
self.assertEqual('version', info.version)

The main benefit to consistency is that when we get a test failure, it
is a bit clearer what the expected outcome is. (We aren't 100%
consistent, but I can see the code snippet just above is doing it, so we
should try to continue.)


A slightly bigger issue is this:
=== modified file 'bzrlib/builtins.py'
- --- bzrlib/builtins.py	2009-09-16 02:52:14 +0000
+++ bzrlib/builtins.py	2009-09-23 07:48:30 +0000
@@ -3242,19 +3242,19 @@
     :Examples:
         Show the current aliases::

- -            bzr alias
+            %(program-name)s alias

         Show the alias specified for 'll'::

- -            bzr alias ll
+            %(program-name)s alias ll

         Set an alias for 'll'::

- -            bzr alias ll="log --line -r-10..-1"
+            %(program-name)s alias ll="log --line -r-10..-1"

         To remove an alias for 'll'::

- -            bzr alias --remove ll
+            %(program-name)s alias --remove ll


^- I understand why you want to do it. However, it feels harder to write
and maintain our own bzrlib code to have to put %(program-name)
everywhere. And it is somewhat likely to regress for you, since we could
easily add another help line anywhere in the program and not realize the
need to %(program-name)s escape it.

My *personal* feeling is that re-using the Command infrastructure is
encouraged and patches to make that easier are welcome.

Reusing an individual cmd_xxx class is allowed, but it doesn't make a
lot of sense to make special provisions for. Certainly you could just
subclass and override the docstring:

class cmd_alias(bzrlib.builtins.cmd_alias):
  """New doc string with my program name."""

You'll likely need to do the inheritance anyway, to inject whatever
changes you need to do to get the aliases to show up in your config
file, rather than in ours.

So *I* personally don't like the change from "bzr => %(program-name)s"
in the help text. I'm not going to veto it, but I'd at least like us to
discuss it before we merge it.

I'm fine with the other changes.

  review: needs_fixing

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkq6LoIACgkQJdeBCYSNAAMdYwCfUqpxVSuKvAT1xAoTei4UgGVn
BaIAn0JxkjqLoDi4PwuaSV8nCfo4Kr+z
=/XhQ
-----END PGP SIGNATURE-----