On Jan 16, 2009, at 09:10 , Curtis Hovey wrote:

> Review: Needs Fixing code
> Hi Brad
>
> Thanks for adding the samples, I'm sure they will help people  
> understand how to use launchapd lib. Your XSLT rule is broken; I  
> suggest a fix.

Thanks for catching that dumb XSLT problem.

>
>
>> === modified file 'launchpadlib/wadl-to-refhtml.xsl'
>> --- launchpadlib/wadl-to-refhtml.xsl	2009-01-13 17:30:59 +0000
>> +++ launchpadlib/wadl-to-refhtml.xsl	2009-01-15 22:34:48 +0000
>> @@ -379,6 +379,12 @@
>>                 <xsl:text>/+wikiname/</xsl:text>
>>                 <var>&lt;id&gt;</var>
>>             </xsl:when>
>> +            <xsl:when test="@id = 'commercial_subscription'">
>> +                <xsl:text>/</xsl:text>
>> +                <xsl:text>/+commercialsubscription/</xsl:text>
>> +                <xsl:text>/</xsl:text>
>> +                <var>&lt;commercial_subscription.id&gt;</var>
>> +            </xsl:when>
>
> This instruction builds a path by concatenating text, and it does  
> not look
> correct. I see
>
>    //+commercialsubscription//<commercial_subscription.id>
>
> /me looks at source
> Yep, I am right. Since the convention is to have wrap the view name in
> "/"s, this markup should look like the h_w_device rules
>
>            <xsl:when test="@id = 'commercial_subscription'">
>                <xsl:text>/+commercialsubscription/</xsl:text>
>                <var>&lt;commercial_subscription.id&gt;</var>
>            </xsl:when>
>

Fixed

>> === added directory 'samples'
>> === added file 'samples/_pythonpath.py'
>> --- samples/_pythonpath.py	1970-01-01 00:00:00 +0000
>> +++ samples/_pythonpath.py	2009-01-15 22:34:48 +0000
>> @@ -0,0 +1,14 @@
>> +__metaclass__ = type
>> +
>> +import sys, os, os.path
>
> Put each import on its own line.

Fixed

>
>
>> +
>> +# See if we can find launchpadlib and waddlib
>> +try:
>> +    import launchpadlib
>> +    import wadllib
>> +except ImportError:
>> +    # Modify sys.path to add trunk/lib.
>> +    HOME = os.environ['HOME']
>> +    trunk_path = os.environ.get('LP_TRUNK_PATH',
>> +                                os.path.join(HOME, 'canonical/lp- 
>> branches/trunk'))
>
> Wrap the code at 78 characters.

Done

>
>
> ...
>
>> === added file 'samples/lpapi.py'
>> --- samples/lpapi.py	1970-01-01 00:00:00 +0000
>> +++ samples/lpapi.py	2009-01-15 22:34:48 +0000
>> @@ -0,0 +1,108 @@
>> +#!/usr/bin/python2.4
>> +import os
>> +import sys
>> +from urlparse import urljoin
>> +import commands
>> +
>> +try:
>> +    from launchpadlib.launchpad import (
>> +        Launchpad, EDGE_SERVICE_ROOT, STAGING_SERVICE_ROOT)
>> +    from launchpadlib.credentials import Credentials
>> +    from launchpadlib.errors import *
>> +    import launchpadlib
>> +except ImportError:
>> +    print >> sys.stderr, "Usage:"
>> +    print >> sys.stderr, "   PYTHONPATH=somebranch/lib %s" %  
>> sys.argv[0]
>> +    raise
>> +
>> +
>> +class InvalidCredentials(Exception):
>> +    pass
>> +
>> +
>> +class Retry(Exception):
>> +    pass
>> +
>> +
>> +class LPSystem:
>
> Can you add docstrings to these. This is a great base class and  
> exceptions
> that I expect other people will use and evolve.'

Yes.  Done.  Retry removed as it was unused.

>
>
>
>> +    endpoint = None
>> +    auth_file = None
>> +    def __init__(self, app_name='just testing', use_cache=True):
>> +        home = os.environ['HOME']
>> +        if use_cache:
>> +            cachedir = os.path.join(home, '.launchpadlib', 'cache')
>> +            if not os.path.exists(cachedir):
>> +                cachedir = None
>> +        else:
>> +            cachedir = None
>
> Is ``cachedir`` right? ``cache_dir`` looks PEP-8 friendly.

Fixed

>
>
>> +        try:
>> +            # Load credentials from AUTH_FILE if it exists.
>> +            self.auth_file = os.path.join(home, self.auth_file_name)
>> +            self.credentials = Credentials()
>> +            self.credentials.load(open(self.auth_file))
>> +            print >> sys.stderr, "Loading credentials..."
>> +            try:
>> +                self.launchpad = Launchpad(self.credentials,  
>> self.endpoint,
>> +                                           cache=cachedir)
>> +            except launchpadlib.errors.HTTPError:
>> +                raise InvalidCredentials, (
>> +                    "Please remove %s and rerun %s to  
>> authenticate." % (
>> +                    self.auth_file, sys.argv[0]))
>> +        except IOError:
>> +            # Prompt for authentication process, then save  
>> credentials to AUTH_FILE.
>
> Wrap the code at 78 characters.

Done

>
>
>> +            try:
>> +                self.launchpad =  
>> Launchpad.get_token_and_login(app_name,
>> +                                                                
>> self.endpoint,
>> +                                                                
>> cache=cachedir)
>> +                 
>> self.launchpad.credentials.save(open(self.auth_file, "w"))
>> +                print >> sys.stderr, "Credentials saved"
>> +            except launchpadlib.errors.HTTPError, err:
>> +                print >> sys.stderr, err.content
>> +                raise
>> +
>> +    @property
>> +    def url(self):
>> +        return urljoin(self.endpoint, 'projects/')
>> +
>> +
>> +class Edge(LPSystem):
>> +    endpoint = EDGE_SERVICE_ROOT
>> +    auth_file_name = 'edge.auth'
>> +
>> +
>> +class Staging(LPSystem):
>> +    endpoint = STAGING_SERVICE_ROOT
>> +    auth_file_name = 'staging.auth'
>> +
>> +
>> +class Production(LPSystem):
>> +    endpoint = 'https://api.launchpad.net/beta/'
>> +    auth_file_name = 'production.auth'
>> +
>> +
>> +class Dev(LPSystem):
>> +    endpoint = 'https://api.launchpad.dev/beta/'
>> +    auth_file_name = 'dev.auth'
>> +    def __init__(self, app_name='just testing'):
>> +        LPSystem.__init__(self, app_name=app_name, use_cache=False)
>> +
>> +
>> +systems=dict(dev=Dev,
>> +             staging=Staging,
>> +             edge=Edge,
>> +             beta=Edge,
>> +             production=Production,
>> +             prod=Production,
>> +             )
>
> Wrap the "=" in spaces:
> systems = dict(dev=Dev,

Done

>
>
> Is beta vestigial? Is it used>

It was just an alias.  Removed.

>
>
>> +
>> +
>> +def lp_factory(sysname, app_name='just_testing'):
>
> I think system_name is clearer

Agreed.

>
>
>> +    """Get a launchpad API instance for `sysname`."""
>
> I believe double-backquotes denotes a variable in ReST. single- 
> backquotes
> is a cross-reference.
>

Good to know.

> ...
>
> === added file 'samples/nopriv-api.py'
> --- samples/nopriv-api.py	1970-01-01 00:00:00 +0000
> +++ samples/nopriv-api.py	2009-01-15 22:34:48 +0000
> @@ -0,0 +1,91 @@
> +#!/usr/bin/python
> +# -*-doctest-*-
> +
> +"""
> +    >>> import _pythonpath, lpapi
> +    >>> lp = lpapi.lp_factory('dev')
> +
> +    >>> bzr = lp.projects['bzr']
> +    >>> print bzr.reviewer_whiteboard
> +    tag:launchpad.net:2008:redacted
> +    >>> bzr.reviewer_whiteboard = "Check on licensing"
> +    >>> print bzr.reviewer_whiteboard
> +    Check on licensing
> +    >>> bzr.lp_save()
> +    ...
> +    Traceback (most recent call last):
> +    ...
> +    HTTPError: HTTP Error 401: Unauthorized
> +
> +    >>> from operator import attrgetter
> +    >>> def print_projs(projs):
> +    ...     for p in sorted(projs, key=attrgetter('name')):
> +    ...         print p.name
>
> Launchpad style to to never use single letters as variables (except  
> for i
> and j).
>
>>>> def print_projects(projects):
>    ...     for project in sorted(projects, key=attrgetter('name')):
>    ...         print project.name
>
> This doesn't look like it is used.

Leftover from c-n-p programming.

>
>
>> +
>> +    >>> inactive = lp.projects.licensing_search(active=False)
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> active = lp.projects.licensing_search(active=True)
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> projs = lp.projects.licensing_search(license_reviewed=False)
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> projs = lp.projects.licensing_search(has_zero_licenses=True)
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> projs = lp.projects.licensing_search(licenses=['Other/ 
>> Proprietary'])
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> bzr.licenses
>> +    []
>> +
>> +    >>> projs =  
>> lp.projects.licensing_search(has_zero_licenses=False)
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> projs = lp.projects.licensing_search(licenses=['GNU Affero  
>> GPL v3'])
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> projs =  
>> lp.projects.licensing_search(subscription_expires_after="2005-01-01")
>
> Wrap the code at 78 characters.

Done

>
>
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> projs =  
>> lp.projects.licensing_search(has_zero_licenses=True,  
>> search_text="Bazaar")
>
> Wrap the code at 78 characters.

Done

>
>
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +"""
>> +
>> +if __name__ == '__main__':
>> +    # Remove existing credentials.
>> +    import os
>> +    try:
>> +        os.unlink(os.path.join(os.environ['HOME'], 'dev.auth'))
>> +    except OSError:
>> +        pass
>> +
>> +    # Create correct credentials.
>> +    print "Login as '<email address hidden>' in your browser."
>> +    print "Press <Enter> when done."
>> +    raw_input()
>> +
>> +    import _pythonpath, lpapi
>
> I think a judicious comment is need to explain that those two imported
> modules setup the scripting environment. It doesn't look like this  
> uses
> _pythonpath, but we know it does.

Right.  Done.

>
>
> ...
>
>> === added file 'samples/sample-person-api.py'
>> --- samples/sample-person-api.py	1970-01-01 00:00:00 +0000
>> +++ samples/sample-person-api.py	2009-01-15 22:34:48 +0000
>> @@ -0,0 +1,91 @@
>> +#!/usr/bin/python
>> +# -*-doctest-*-
>> +
>> +"""
>> +    >>> import _pythonpath, lpapi
>> +    >>> lp = lpapi.lp_factory('dev')
>> +
>> +    >>> bzr = lp.projects['bzr']
>> +    >>> print bzr.reviewer_whiteboard
>> +    tag:launchpad.net:2008:redacted
>> +    >>> bzr.reviewer_whiteboard = "Check on licensing"
>> +    >>> print bzr.reviewer_whiteboard
>> +    Check on licensing
>> +    >>> bzr.lp_save()
>> +    ...
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> from operator import attrgetter
>> +    >>> def print_projs(projs):
>> +    ...     for p in sorted(projs, key=attrgetter('name')):
>> +    ...         print p.name
>
> This looks like the helper from the previous test. It should be the  
> the
> same as I suggested above, or extracted to a testing module that can
> be imported. Oh and this too does not look like it is used.

Removed.

This function is actually used in another test which I forgot to  
include in the branch.  That test has been added and all of the  
comments made here addressed in it too.  Sorry for the oversight.

>
>
>> +
>> +    >>> inactive = lp.projects.licensing_search(active=False)
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> active = lp.projects.licensing_search(active=True)
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> projs = lp.projects.licensing_search(license_reviewed=False)
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> projs = lp.projects.licensing_search(has_zero_licenses=True)
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> projs = lp.projects.licensing_search(licenses=['Other/ 
>> Proprietary'])
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> bzr.licenses
>> +    []
>> +
>> +    >>> projs =  
>> lp.projects.licensing_search(has_zero_licenses=False)
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> projs = lp.projects.licensing_search(licenses=['GNU Affero  
>> GPL v3'])
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> projs =  
>> lp.projects.licensing_search(subscription_expires_after="2005-01-01")
>
> Wrap the code at 78 characters.

Done

>
>
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +    >>> projs =  
>> lp.projects.licensing_search(has_zero_licenses=True,  
>> search_text="Bazaar")
>
> Wrap the code at 78 characters.

Done

>
>
>> +    Traceback (most recent call last):
>> +    ...
>> +    HTTPError: HTTP Error 401: Unauthorized
>> +
>> +"""
>> +
>> +if __name__ == '__main__':
>> +    # Remove existing credentials.
>> +    import os
>> +    try:
>> +        os.unlink(os.path.join(os.environ['HOME'], 'dev.auth'))
>> +    except OSError:
>> +        pass
>> +
>> +    # Create correct credentials.
>> +    print "Login as '<email address hidden>' in your browser."
>> +    print "Press <Enter> when done."
>> +    raw_input()
>> +
>> +    import _pythonpath, lpapi
>
> This too is doing something magical that a good comment would  
> elucidate.

Done

>
>
> ...
>
>> === modified file 'setup.py'
>> --- setup.py	2008-08-01 16:05:31 +0000
>> +++ setup.py	2009-01-15 22:34:48 +0000
>
> ...
>
> -- 
> https://code.edge.launchpad.net/~bac/launchpadlib/lplib-comm_sub/ 
> +merge/2866
> You are subscribed to branch lp:~bac/launchpadlib/lplib-comm_sub.