Merge lp:~jkakar/bzr/custom-project-name into lp:bzr

Proposed by Jamu Kakar
Status: Work in progress
Proposed branch: lp:~jkakar/bzr/custom-project-name
Merge into: lp:bzr
Diff against target: 98 lines (+42/-3)
3 files modified
bzrlib/__init__.py (+3/-0)
bzrlib/commands.py (+5/-3)
bzrlib/tests/test_help.py (+34/-0)
To merge this branch: bzr merge lp:~jkakar/bzr/custom-project-name
Reviewer Review Type Date Requested Status
Martin Pool Needs Fixing
John A Meinel Needs Fixing
Review via email: mp+12267@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Jamu Kakar (jkakar) wrote :

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.

Revision history for this message
John A Meinel (jameinel) wrote :
Download full text (4.9 KiB)

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

Read more...

review: Needs Fixing
Revision history for this message
John A Meinel (jameinel) wrote :

Martin- I asked for your specific review to get another opinion on changing help text for stuff like "cmd_alias" to be:

 %(program-name)s alias ...

from
 bzr alias ...

lp:~jkakar/bzr/custom-project-name updated
4724. By Jamu Kakar

- Cleanup stylist problems pointed out by jam.
- Use bzrlib.version_string instead of bzrlib.__version__.

4725. By Jamu Kakar

- Merged trunk.

Revision history for this message
Jamu Kakar (jkakar) wrote :

> There are a few stylistic things that we generally prefer. For example:
> +def set_program_info(info):
>
> ^- two blank lines before 'set'

Thanks, fixed.

> + 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)

Thanks, fixed.

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

Yeah, this change isn't 100% necessary and I can totally understand
why you're unhappy with it. What I do right now in Commandant is
s/bzr/my-program-name/ before using content produced by
Command.get_help_text and that works fine.

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

What I'm hoping to do is make the configuration system in bzrlib
pluggable. I'd prefer not to reimplement cmd_alias if possible.
All I really need to do is for bzrlib.config.config_filename to
return a path to my config file instead of ~/.bazaar/bazaar.conf and
it'll just work.

> 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 don't have a strong opinion about the change. If it feels ugly
I'd be happy to back it out. We can always introduce it again in
the future if we decide it makes sense to have.

Thanks for the review!

Revision history for this message
Robert Collins (lifeless) wrote :

I have only vicariously reviewed so far (by reading John's review). I'll
do a deeper review after these issues are sorted.

firstly I don't think we need indirection on the ProgramInfo

statically initialise it

program_info = ProgramInfo()

And if someone needs to replace it they can. See ui.ui_factory and how
its managed to see this in practice today.

On Wed, 2009-09-23 at 14:21 +0000, John A Meinel wrote:
> 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 too have a concern about this. I think its ok for a small number of
commands, but for many it would be very tedious - and I really don't
know how many are even vaguely suitable to be used verbatim.

-Rob

Revision history for this message
Martin Pool (mbp) wrote :

2009/9/24 John A Meinel <email address hidden>:
> Martin- I asked for your specific review to get another opinion on changing help text for stuff like "cmd_alias" to be:
>
>  %(program-name)s alias ...

In general I think separating infrastructure from bazaar-specific use
of it is good.

I think the help text is fine. I would prefer and recommend that the
variables used in the dict be valid identifiers in python, ie
program_name not a hyphen. It doesn't make a big difference in this
patch but in general it helps that you can pass them as kwargs etc.

It seems like there should be a brief description in the developer
docs of why we do this.

This text is inserted into the autogenerated user reference so you
should check that the substitution is done correctly there.

The ProgramInfo stuff is a bit distressingly large for what amounts to
two global variables set at startup time to constant values. (Isn't
it?) It's more like a Java style, where in Python it would be more
idiomatic and simple to just have a global variable that either holds
the value or a callable for the value...

I wonder if this will break any plugins or commands that happen to
have % is their help string?

I don't think this would be worth the risk for 2.0.

--
Martin <http://launchpad.net/~mbp/>

Revision history for this message
Martin Pool (mbp) wrote :

(comments by mail)

review: Needs Fixing
Revision history for this message
Vincent Ladeuil (vila) wrote :

>>>>> "martin" == Martin Pool <email address hidden> writes:

    martin> 2009/9/24 John A Meinel <email address hidden>:
    >> Martin- I asked for your specific review to get another opinion on changing help text for stuff like "cmd_alias" to be:
    >>
    >>  %(program-name)s alias ...

optparse uses 'prog' (arguably %prog without parens, but yet)
which seems clear enough to me and avoid the '-' vs '_' issue.

Revision history for this message
Martin Pool (mbp) wrote :

Thinking about this a bit more:

There are other parts of bzrlib, like the UIFactory, test runner or
the apport integration, that could potentially be reused in other
places and that may need to report the program name and version
number. The user-agent string sent by transports is another case.

Therefore I think bzr globally needs a concept of "what program is
using me, and what is its version?" It just so happens that this is
normally the script called bzr from the same tree as bzrlib, but
that's only the default.

This should be global to bzrlib. Arguably it should be global to the
Python interpreter instance, but I can't think of a good or standard
place to put it.

So perhaps the most appropriate thing is to just have
bzrlib.application_name and application_version and/or
application_version_tuple. They can be initialized to 'bzr' and the
same as the library version, but could be overridden after importing.
(Even the bzr script could do this...)

--
Martin <http://launchpad.net/~mbp/>

lp:~jkakar/bzr/custom-project-name updated
4726. By Jamu Kakar

- Merged trunk.

4727. By Jamu Kakar

- Revert changes to cmd_alias docstring.

4728. By Jamu Kakar

- Merged trunk.

4729. By Jamu Kakar

- Removed help variable substitution as it wasn't fully baked.
- Replaced ProgramInfo and related functions with a simple
  bzrlib.application_name attribute, defaulting to 'bzr'.

4730. By Jamu Kakar

- Added a test and updated one more spot where 'bzr' was hard-coded.

Revision history for this message
Jamu Kakar (jkakar) wrote :

Thanks a lot for the feedback and sorry for taking a while to
respond. I've made the following changes to the branch:

- bzrlib.application_name defaults to 'bzr' and can be overridden by
  programs using bzrlib.

- Variable substitution logic for help topics has been removed.

- bzrlib.application_name is used by bzrlib.commands when generating
  help output.

I thought about adding application_version as suggested, but I
decided to leave it out for the time being since I wasn't changing
anything that would use it and version_string is already present.

lp:~jkakar/bzr/custom-project-name updated
4731. By Jamu Kakar

- Minor formatting improvements.

4732. By Jamu Kakar

- Merged trunk.

Unmerged revisions

4732. By Jamu Kakar

- Merged trunk.

4731. By Jamu Kakar

- Minor formatting improvements.

4730. By Jamu Kakar

- Added a test and updated one more spot where 'bzr' was hard-coded.

4729. By Jamu Kakar

- Removed help variable substitution as it wasn't fully baked.
- Replaced ProgramInfo and related functions with a simple
  bzrlib.application_name attribute, defaulting to 'bzr'.

4728. By Jamu Kakar

- Merged trunk.

4727. By Jamu Kakar

- Revert changes to cmd_alias docstring.

4726. By Jamu Kakar

- Merged trunk.

4725. By Jamu Kakar

- Merged trunk.

4724. By Jamu Kakar

- Cleanup stylist problems pointed out by jam.
- Use bzrlib.version_string instead of bzrlib.__version__.

4723. By Jamu Kakar

- Command.get_help_variables can be overridden to provide
  command-level custom help variables.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/__init__.py'
2--- bzrlib/__init__.py 2009-11-16 19:45:47 +0000
3+++ bzrlib/__init__.py 2009-11-24 19:19:10 +0000
4@@ -35,6 +35,9 @@
5 IGNORE_FILENAME = ".bzrignore"
6
7
8+application_name = "bzr"
9+
10+
11 __copyright__ = "Copyright 2005, 2006, 2007, 2008, 2009 Canonical Ltd."
12
13 # same format as sys.version_info: "A tuple containing the five components of
14
15=== modified file 'bzrlib/commands.py'
16--- bzrlib/commands.py 2009-11-04 22:32:13 +0000
17+++ bzrlib/commands.py 2009-11-24 19:19:10 +0000
18@@ -402,7 +402,8 @@
19
20 Only describes arguments, not options.
21 """
22- s = 'bzr ' + self.name() + ' '
23+ import bzrlib
24+ s = bzrlib.application_name + ' ' + self.name() + ' '
25 for aname in self.takes_args:
26 aname = aname.upper()
27 if aname[-1] in ['$', '+']:
28@@ -490,8 +491,9 @@
29 result += ':%s:\n%s\n' % (label,sections[label])
30 result += '\n'
31 else:
32- result += ("See bzr help %s for more details and examples.\n\n"
33- % self.name())
34+ import bzrlib
35+ result += ("See %s help %s for more details and examples.\n\n"
36+ % (bzrlib.application_name, self.name()))
37
38 # Add the aliases, source (plug-in) and see also links, if any
39 if self.aliases:
40
41=== modified file 'bzrlib/tests/test_help.py'
42--- bzrlib/tests/test_help.py 2009-06-15 10:22:08 +0000
43+++ bzrlib/tests/test_help.py 2009-11-24 19:19:10 +0000
44@@ -18,6 +18,7 @@
45
46 from cStringIO import StringIO
47
48+import bzrlib
49 from bzrlib import (
50 builtins,
51 commands,
52@@ -65,6 +66,23 @@
53 self.assertEndsWith(helptext,
54 ' -h, --help Show help message.\n\n')
55
56+ def test_get_help_text_with_custom_application_name(self):
57+ """
58+ The 'Usage' part of the help text uses the application name registered
59+ by the program using bzrlib.
60+ """
61+ self.addCleanup(setattr, bzrlib, "application_name",
62+ bzrlib.application_name)
63+ bzrlib.application_name = "foo"
64+ class cmd_Demo(commands.Command):
65+ """A sample command."""
66+ cmd = cmd_Demo()
67+ helptext = cmd.get_help_text()
68+ self.assertStartsWith(
69+ 'Purpose: A sample command.\n'
70+ 'Usage: foo Demo',
71+ helptext)
72+
73 def test_command_with_additional_see_also(self):
74 class cmd_WithSeeAlso(commands.Command):
75 """A sample command."""
76@@ -203,6 +221,22 @@
77 'See bzr help Demo for more details and examples.\n'
78 '\n')
79
80+ def test_concise_help_text_with_custom_application_name(self):
81+ """
82+ Concise help text shows information about getting more details, which
83+ includes the application name.
84+ """
85+ self.addCleanup(setattr, bzrlib, "application_name",
86+ bzrlib.application_name)
87+ bzrlib.application_name = "foo"
88+ class cmd_Demo(commands.Command):
89+ """A sample command."""
90+ cmd = cmd_Demo()
91+ helptext = cmd.get_help_text(verbose=False)
92+ self.assertEndsWith('See foo help Demo for more details and examples.\n'
93+ '\n',
94+ helptext)
95+
96 def test_help_custom_section_ordering(self):
97 """Custom descriptive sections should remain in the order given."""
98 class cmd_Demo(commands.Command):