Merge into devel : lplib-comm_sub : Code : launchpadlib

Status:	Merged
Merged at revision:	not available
Proposed branch:	lp:~bac/launchpadlib/lplib-comm_sub
Merge into:	lp:~launchpad-pqm/launchpadlib/devel
To merge this branch:	bzr merge lp:~bac/launchpadlib/lplib-comm_sub
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Curtis Hovey (community)	code	2009-01-14	Approve on 2009-01-17
Launchpad PQM Bot	code	2009-01-14	Pending
Review via email: mp+2866@code.launchpad.net

Revision history for this message

Brad Crittenden (bac) wrote on 2009-01-14:

#

Add XSLT processing for commercial_subscriptions

lp:~bac/launchpadlib/lplib-comm_sub updated on 2009-01-15

29. By Brad Crittenden on 2009-01-15: Added sample scripts.
30. By Brad Crittenden on 2009-01-15: Added helper files.
31. By Brad Crittenden on 2009-01-15: added a new sample test using sample-person

Revision history for this message

Curtis Hovey (sinzui) wrote on 2009-01-16:

#

Download full text (12.5 KiB)

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.

> === 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><id></var>
> </xsl:when>
> + <xsl:when test="@id = 'commercial_subscription'">
> + <xsl:text>/</xsl:text>
> + <xsl:text>/+commercialsubscription/</xsl:text>
> + <xsl:text>/</xsl:text>
> + <var><commercial_subscription.id></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><commercial_subscription.id></var>
            </xsl:when>

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

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

...

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

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

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.

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

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

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

...

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

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

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

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

Is beta vestigial? Is it used>

> +
> +
> +def lp_factory(sysname, app_name='just_testing'):

I think system_name is clearer

> +    """Get a launchpad API instance for `sysname`."""

I believe double-backquotes denotes a variable in ReST. single-backquotes
is a cross-reference.

...

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

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

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

> +    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 'no-priv@canonical.com' 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.

...

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

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

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

> +    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 'test@canonical.com' 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.

...

> === modified file 'setup.py'
> --- setup.py	2008-08-01 16:05:31 +0000
> +++ setup.py	2009-01-15 22:34:48 +0000

...

review: Needs Fixing (code)

lp:~bac/launchpadlib/lplib-comm_sub updated on 2009-01-16

32. By Brad Crittenden on 2009-01-16: Changes per review.
33. By Brad Crittenden on 2009-01-16: Fixed typo

Revision history for this message

Brad Crittenden (bac) wrote on 2009-01-16:

#

Download full text (13.8 KiB)

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><id></var>
>> </xsl:when>
>> + <xsl:when test="@id = 'commercial_subscription'">
>> + <xsl:text>/</xsl:text>
>> + <xsl:text>/+commercialsubscription/</xsl:text>
>> + <xsl:text>/</xsl:text>
>> + <var><commercial_subscription.id></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><commercial_subscription.id></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 ...

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 'no-priv@canonical.com' 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 'test@canonical.com' 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.

Revision history for this message

Curtis Hovey (sinzui) wrote on 2009-01-17:

#

Hi Brad.

Thanks for making these changes. I approve this branch for merging.

review: Approve (code)

lp:~bac/launchpadlib/lplib-comm_sub updated on 2009-01-18

34. By Brad Crittenden on 2009-01-18: Merge from trunk

launchpadlib

Merge lp:~bac/launchpadlib/lplib-comm_sub into lp:~launchpad-pqm/launchpadlib/devel

Commit message

Description of the change

Subscribers