launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/api-docstrings-touchup into lp:maas.

Commit message:
Rearrange maas-cli's --help output, and add to the API docstrings.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/api-docstrings-touchup/+merge/129349

When adding an arguments sub-parser to maas-cli, we had been assuming that we could set the "help" and "description" properties to the subject's docstring title and docstring body.  But it doesn't seem to work that way: the "help" text doesn't show up in the sub-parser's --help output, only in its summary on the parent parser's --help output.  This wasn't very clear because many of our docstrings lacked a body.

In this branch I set both "help" and "description" for a sub-parser to the docstring title, and add the body as an epilog.  That way it shows up at the bottom of the sub-parser's --help output, and the title shows up near the top.  I also added some docstring bodies to the API, mostly to explain arguments that are provided through the URL rather than the request, and hopefully made the titles a bit more consistent.

This is really just the outcome of experimentation and there is no grand driving idea except "make maas-cli --help a bit clearer," so I did not have a pre-implementation call.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/api-docstrings-touchup/+merge/129349
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/api-docstrings-touchup into lp:maas.

=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2012-10-08 20:56:17 +0000
+++ src/maascli/api.py	2012-10-12 05:30:22 +0000
@@ -257,7 +257,8 @@
             }
         action_class = type(action_name, action_bases, action_ns)
         action_parser = parser.subparsers.add_parser(
-            action_name, help=help_title, description=help_body)
+            action_name, help=help_title, description=help_title,
+            epilog=help_body)
         action_parser.set_defaults(
             execute=action_class(action_parser))
 
@@ -267,7 +268,8 @@
     help_title, help_body = parse_docstring(handler["doc"])
     handler_name = handler_command_name(handler["name"])
     handler_parser = parser.subparsers.add_parser(
-        handler_name, help=help_title, description=help_body)
+        handler_name, help=help_title, description=help_title,
+        epilog=help_body)
     register_actions(profile, handler, handler_parser)
 
 
@@ -314,5 +316,17 @@
         for profile_name in config:
             profile = config[profile_name]
             profile_parser = parser.subparsers.add_parser(
-                profile["name"], help="Interact with %(url)s" % profile)
+                profile["name"], help="Interact with %(url)s" % profile,
+                description=(
+                    "Issue commands to the MAAS region controller at %(url)s."
+                    % profile),
+                epilog=(
+                    "This is a profile.  Any commands you issue on this "
+                    "profile will operate on the MAAS region server.\n"
+                    "The command information you see here comes from the "
+                    "region server's API; it may differ for different "
+                    "profiles.  If you believe the API may have changed, "
+                    "use the command's 'refresh' sub-command to fetch the "
+                    "latest version of this help information from the "
+                    "server."))
             register_resources(profile, profile_parser)

=== modified file 'src/maascli/cli.py'
--- src/maascli/cli.py	2012-10-09 11:54:44 +0000
+++ src/maascli/cli.py	2012-10-12 05:30:22 +0000
@@ -29,7 +29,7 @@
 
 
 class cmd_login(Command):
-    """Log-in to a remote API, storing its description and credentials.
+    """Log in to a remote API, and remember its description and credentials.
 
     If credentials are not provided on the command-line, they will be prompted
     for interactively.
@@ -95,7 +95,12 @@
 
 
 class cmd_refresh(Command):
-    """Refresh the API descriptions of all profiles."""
+    """Refresh the API descriptions of all profiles.
+
+    This retrieves the latest version of the help information for each
+    profile.  Use it to update your maas-cli client's information after an
+    upgrade to the MAAS server.
+    """
 
     def __call__(self, options):
         with ProfileConfig.open() as config:
@@ -107,7 +112,11 @@
 
 
 class cmd_logout(Command):
-    """Log-out of a remote API, purging any stored credentials."""
+    """Log out of a remote API, purging any stored credentials.
+
+    This will remove the given profile from your maas-cli client.  You can
+    re-create it by logging in again later.
+    """
 
     def __init__(self, parser):
         super(cmd_logout, self).__init__(parser)
@@ -151,5 +160,6 @@
     for name, command in commands.items():
         help_title, help_body = parse_docstring(command)
         command_parser = parser.subparsers.add_parser(
-            safe_name(name), help=help_title, description=help_body)
+            safe_name(name), help=help_title, description=help_title,
+            epilog=help_body)
         command_parser.set_defaults(execute=command(command_parser))

=== modified file 'src/maascli/utils.py'
--- src/maascli/utils.py	2012-10-08 20:56:17 +0000
+++ src/maascli/utils.py	2012-10-12 05:30:22 +0000
@@ -39,6 +39,11 @@
 
 
 def parse_docstring(thing):
+    """Parse python docstring for `thing`.
+
+    Returns a tuple: (title, body).  As per docstring conventin, title is the
+    docstring's first paragraph and body is the rest.
+    """
     is_string = isinstance(thing, basestring)
     doc = cleandoc(thing) if is_string else getdoc(thing)
     doc = empty if doc is None else doc

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-11 13:55:44 +0000
+++ src/maasserver/api.py	2012-10-12 05:30:22 +0000
@@ -467,7 +467,10 @@
 
 
 class NodeHandler(OperationsHandler):
-    """Manage individual Nodes."""
+    """Manage an individual Node.
+
+    The Node is identified by its system_id.
+    """
     create = None  # Disable create.
     model = Node
     fields = DISPLAYED_NODE_FIELDS
@@ -643,7 +646,7 @@
 
 
 class AnonNodesHandler(AnonymousOperationsHandler):
-    """Create Nodes."""
+    """Anonymous access to Nodes."""
     create = read = update = delete = None
     fields = DISPLAYED_NODE_FIELDS
 
@@ -715,7 +718,7 @@
 
 
 class NodesHandler(OperationsHandler):
-    """Manage collection of Nodes."""
+    """Manage the collection of all Nodes in the MAAS."""
     create = read = update = delete = None
     anonymous = AnonNodesHandler
 
@@ -851,10 +854,12 @@
 
 
 class NodeMacsHandler(OperationsHandler):
-    """
-    Manage all the MAC addresses linked to a Node / Create a new MAC address
-    for a Node.
-
+    """Manage MAC addresses for a given Node.
+
+    This is where you manage the MAC addresses linked to a Node, including
+    associating a new MAC address with the Node.
+
+    The Node is identified by its system_id.
     """
     update = delete = None
 
@@ -878,7 +883,11 @@
 
 
 class NodeMacHandler(OperationsHandler):
-    """Manage a MAC address linked to a Node."""
+    """Manage a MAC address.
+
+    The MAC address object is identified by the system_id for the Node it
+    is attached to, plus the MAC address itself.
+    """
     create = update = None
     fields = ('mac_address',)
     model = MACAddress
@@ -995,7 +1004,7 @@
 
 
 class AnonNodeGroupsHandler(AnonymousOperationsHandler):
-    """Anon Node-groups API."""
+    """Anonymous access to NodeGroups."""
     create = read = update = delete = None
     fields = DISPLAYED_NODEGROUP_FIELDS
 
@@ -1088,7 +1097,7 @@
 
 
 class NodeGroupsHandler(OperationsHandler):
-    """Node-groups API."""
+    """Manage NodeGroups."""
     anonymous = AnonNodeGroupsHandler
     create = read = update = delete = None
     fields = DISPLAYED_NODEGROUP_FIELDS
@@ -1157,7 +1166,17 @@
 
 
 class NodeGroupHandler(OperationsHandler):
-    """Node-group API."""
+    """Manage a NodeGroup.
+
+    NodeGroup is the internal name for a cluster.
+
+    The NodeGroup is identified by its uuid, a random identifier that looks
+    something like:
+
+        5977f6ab-9160-4352-b4db-d71a99066c4f
+
+    Each NodeGroup has its own uuid.
+    """
 
     create = update = delete = None
     fields = DISPLAYED_NODEGROUP_FIELDS
@@ -1176,6 +1195,11 @@
 
     @operation(idempotent=False)
     def update_leases(self, request, uuid):
+        """Submit latest state of DHCP leases within the cluster.
+
+        The cluster controller calls this periodically to tell the region
+        controller about the IP addresses it manages.
+        """
         leases = get_mandatory_param(request.data, 'leases')
         nodegroup = get_object_or_404(NodeGroup, uuid=uuid)
         check_nodegroup_access(request, nodegroup)
@@ -1234,7 +1258,11 @@
 
 
 class NodeGroupInterfacesHandler(OperationsHandler):
-    """NodeGroupInterfaces API."""
+    """Manage NodeGroupInterfaces.
+
+    A NodeGroupInterface is a network interface attached to a cluster
+    controller, with its network properties.
+    """
     create = read = update = delete = None
     fields = DISPLAYED_NODEGROUP_FIELDS
 
@@ -1283,7 +1311,11 @@
 
 
 class NodeGroupInterfaceHandler(OperationsHandler):
-    """NodeGroupInterface API."""
+    """Manage a NodeGroupInterface.
+
+    A NodeGroupInterface is identified by the uuid for its NodeGroup, and
+    the name of the network interface it represents: "eth0" for example.
+    """
     create = delete = None
     fields = DISPLAYED_NODEGROUP_FIELDS
 
@@ -1386,7 +1418,13 @@
 
 
 class TagHandler(OperationsHandler):
-    """Manage individual Tags."""
+    """Manage individual Tags.
+
+    Tags are properties that can be associated with a Node and serve as
+    criteria for selecting and allocating nodes.
+
+    A Tag is identified by its name.
+    """
     create = None
     model = Tag
     fields = (