Merge lp:~clint-fewbar/pyjuju/add-version-arg into lp:pyjuju

[Clint Byrum]

Add --version option

https://codereview.appspot.com/6593044/

[Clint Byrum (clint-fewbar)]

Reviewers: mp+127109_code.launchpad.net,

Message:
Please take a look.

Description:
Add --version option

https://code.launchpad.net/~clint-fewbar/juju/add-version-arg/+merge/127109

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6593044/

Affected files:
   A [revision details]
   M juju/control/__init__.py
   M juju/control/tests/test_control.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: juju/control/__init__.py
=== modified file 'juju/control/__init__.py'
--- juju/control/__init__.py 2012-03-30 19:12:05 +0000
+++ juju/control/__init__.py 2012-09-29 16:31:24 +0000
@@ -6,6 +6,7 @@
  from .command import Commander
  from .utils import ParseError
  from juju.environment.config import EnvironmentsConfig
+from juju import __version__

  import add_relation
  import add_unit
@@ -106,6 +107,9 @@
          help="Enable verbose logging")

      parser.add_argument(
+ "--version", action="version", version='juju %s' % (__version__))
+
+ parser.add_argument(
          "--log-file", "-l", default=sys.stderr,
type=argparse.FileType('a'),
          help="Log output to file")

Index: juju/control/tests/test_control.py
=== modified file 'juju/control/tests/test_control.py'
--- juju/control/tests/test_control.py 2012-05-04 22:43:40 +0000
+++ juju/control/tests/test_control.py 2012-09-29 16:31:24 +0000
@@ -14,6 +14,7 @@
  from juju.state.tests.common import StateTestBase

  from juju.lib.testing import TestCase
+from juju import __version__

  from .common import ControlToolTest

@@ -62,6 +63,18 @@
          self.assertIn("destroy-environment", output)
          self.assertIn("juju cloud orchestration admin", output)

+ def test_version(self):
+ stderr = self.capture_stream("stderr")
+ try:
+ main(['--version'])
+ except SystemExit, e:
+ self.assertEqual(e.args[0], 0)
+ else:
+ self.fail("Should have exited")
+
+ output = stderr.getvalue()
+ self.assertIn(__version__, output)
+
      def test_custom_parser_does_not_extend_to_subcommand(self):
          stderr = self.capture_stream("stderr")
          self.mocker.replay()

[Jim Baker (jimbaker)]

LGTM with the minor. One thing I noticed is that I would expect the
output on stdout, not stderr, so that I can do this

export JUJU_VERSION=$(juju --version)

instead of

export JUJU_VERSION=$(juju --version 2>1)

Of course this behavior of using stderr is just what argparse does.
However, in a completely random sampling of commands, most print to
stdout (ls, xargs, bzr). One notable exception: python, it uses stderr.
Same with java -version.

However, supporting --version like this is going to have to work around
the fact that we use argparse to support subcommands. And anything
dealing with subcommands in argparse is possible, but way too
complicated.

So, never mind, let's just do what argparse makes easy :)

https://codereview.appspot.com/6593044/diff/1/juju/control/__init__.py
File juju/control/__init__.py (right):

https://codereview.appspot.com/6593044/diff/1/juju/control/__init__.py#newcode110
juju/control/__init__.py:110: "--version", action="version",
version='juju %s' % (__version__))
version="juju %s" % __version__

would be preferable (we use double quotes uniformly in the Juju
codebase).

https://codereview.appspot.com/6593044/

=== modified file 'juju/control/__init__.py'
--- juju/control/__init__.py	2012-03-30 19:12:05 +0000
+++ juju/control/__init__.py	2012-09-29 16:34:21 +0000
@@ -6,6 +6,7 @@
 from .command import Commander
 from .utils import ParseError
 from juju.environment.config import EnvironmentsConfig
+from juju import __version__
 
 import add_relation
 import add_unit
@@ -106,6 +107,9 @@
         help="Enable verbose logging")
 
     parser.add_argument(
+        "--version", action="version", version='juju %s' % (__version__))
+
+    parser.add_argument(
         "--log-file", "-l", default=sys.stderr, type=argparse.FileType('a'),
         help="Log output to file")
 

=== modified file 'juju/control/tests/test_control.py'
--- juju/control/tests/test_control.py	2012-05-04 22:43:40 +0000
+++ juju/control/tests/test_control.py	2012-09-29 16:34:21 +0000
@@ -14,6 +14,7 @@
 from juju.state.tests.common import StateTestBase
 
 from juju.lib.testing import TestCase
+from juju import __version__
 
 from .common import ControlToolTest
 
@@ -62,6 +63,18 @@
         self.assertIn("destroy-environment", output)
         self.assertIn("juju cloud orchestration admin", output)
 
+    def test_version(self):
+        stderr = self.capture_stream("stderr")
+        try:
+            main(['--version'])
+        except SystemExit, e:
+            self.assertEqual(e.args[0], 0)
+        else:
+            self.fail("Should have exited")
+
+        output = stderr.getvalue()
+        self.assertIn(__version__, output)
+
     def test_custom_parser_does_not_extend_to_subcommand(self):
         stderr = self.capture_stream("stderr")
         self.mocker.replay()