Also, flake8 the script. I'm sure I've missed something. Also feel
free to pip install flake8-docstrings and re-flake the script.

On Wed, Mar 18, 2015 at 5:09 PM, Zygmunt Krynicki
<email address hidden> wrote:
> Review: Needs Fixing
>
> Hey.
>
> Have a look at my comments below
>
> Diff comments:
>
>> === added file 'certification_reports/get_ytd_makes.py'
>> --- certification_reports/get_ytd_makes.py    1970-01-01 00:00:00 +0000
>> +++ certification_reports/get_ytd_makes.py    2015-03-18 15:57:45 +0000
>> @@ -0,0 +1,131 @@
>> +#!/usr/bin/python2
>
> for all python2 code please do at a bare minimum:
>
> from __future__ import print_function, absolute_import
>
>> +'''
>> +Script to summarize make and model certification for the year to date
>
> Please move this to the description= keyword argument of the argparse.ArgumentParser() below
>
>> +or for a specified year. Produces a list of makes, the number of
>> +certificates issued for each make, and a list of the models certified
>> +for that make.
>> +
>> +Options:
>> +- C3 username
>> +- C3 API key (from https://certification.canonical.com/me/)
>> +- Ubuntu release (e.g., "precise" or "trusty")
>> +- Year (e.g., "2015")
>> +- --certnum={value} -- Certificate number for a report on one machine or
>> +    "all" for all certified systems. Defaults to "all". Used for debugging,
>> +    since retrieving all certificates takes ~10 minutes.
>> +
>> +Copyright (c) 2015 Canonical Ltd.
>
> please move the copyright to a comment at the top of the file
>
>> +
>> +Author: Rod Smith <email address hidden>
>> +'''
>> +
>> +from api_utils import APIQuery, QueryError
>
> Please put imports in this order: stdlib, 3rd party, this app/library. Put a newline between each group
>
>> +from argparse import ArgumentParser
>> +
>> +import sys
>> +import time
>> +
>> +C3_URL = "https://certification.canonical.com"
>> +CERTIFICATE_API = C3_URL + "/api/v1/certificates"
>> +
>> +SERVER_FF = ['Expansion Chassis',
>> +             'Main Server Chassis',
>> +             'Multi-system',
>> +             'Blade',
>> +             'Rack Mount Chassis',
>> +             'Server']
>> +
>> +api = APIQuery(C3_URL)
>> +
>> +def get_server_certs(username, api_key, release, year, certnum):
>> +    '''
>> +    Retrieve certificate information from C3
>
> Please follow PEP for docstrings
>
> Retrieve certificate information from C3.
>
> :param username:
>    Bla bla bla
> :returns:
>    Dictionary with ...
>
>> +    Return value: Dictionary with manufacturer names as keys and
>> +                  list of models as values
>> +    '''
>> +    request_url = CERTIFICATE_API
>> +    summaries = {}
>> +
>> +    if certnum == "all":
>> +        request_params = {"username": username,
>> +                          "api_key": api_key}
>> +    else:
>> +        request_params = {"username": username,
>> +                          "api_key": api_key,
>> +                          "name": certnum}
>> +
>> +    certs = []
>> +    try:
>> +        certs = api.batch_query(request_url, params=request_params)
>> +    except QueryError:
>> +        print "Unable to get certificates"
>
> you can raise SystemExit("Unable to gett...."), this will behave better than printing this manually. You also don't need the sys.exit() call anymore
>
>> +        sys.exit(1)
>> +    certs = [cert for cert in certs if cert['name']]
>> +
>> +    for cert in certs:
>> +        if cert['created_at'][0:4] == year:
>> +            machine = api.single_query(C3_URL + cert['machine'],
>> +                                       params=request_params)
>> +            if (machine['platform']['form_factor'] in SERVER_FF and
>> +                    cert['release']['codename'] == release):
>> +                make = machine['account']['name']
>> +                details = {'model': machine['platform']['name'],
>> +                           'certnum': cert['name'],
>> +                           'cid': machine['canonical_id'].encode('utf-8')}
>> +                try:
>> +                    summaries[make].append(details)
>> +                except:
>
> Please never use blank except lists. What are you expecting to catch here?
>
>> +                    summaries[make] = [details]
>> +
>> +    return summaries
>> +
>> +
>> +def main():
>> +    parser = ArgumentParser(description="A YTD report on certificates " +
>
> It's my personal preference but I'd rather format this whole section like this:
>
> parser = argparse.ArgumentParser(
>    ...)
> parser.add_argument(
>    '...', foo='...',
>    bar='...')
> ...
>
> This is more consistent as typically all of this is wrong and wraps poorly
>
>> +                            "issued, by make.")
>> +    parser.add_argument('username',
>> +                        help="Launchpad username used to access C3.")
>> +    parser.add_argument('api_key',
>> +                        help="API key used to access C3.")
>> +    parser.add_argument("release", help="Ubuntu release ('trusty', " +
>> +                        "'precise', etc.)")
>> +    parser.add_argument("year", help="The year to summarize")
>> +    parser.add_argument("--certnum", help="Certificate number or 'all'" +
>> +                        "(used for debugging)", default="all")
>> +    args = parser.parse_args()
>> +
>> +    summaries = get_server_certs(args.username, args.api_key, args.release,
>> +                                 args.year, args.certnum)
>> +
>> +    print "YTD server certificates for " + args.release + " in " + args.year
>
> please use str.format() for that
>
>> +    print "\nReport generated: %s\n"  % time.strftime("%c")
>> +
>> +    # First pass: Generate "executive summary"
>> +    print "Summary:"
>> +    print "--------\n"
>> +    grand_total = 0
>> +    for make in sorted(summaries):
>> +        num_of_make = 0
>> +        for model in sorted(summaries[make]):
>
> If distinct values of "make" repeat at all I'd rather keep a cached copy of sorted(summaries[make]) to lower complexity. You only need to calculate that if make changes from last_make
>
>> +            num_of_make += 1
>> +            grand_total += 1
>> +        print make.encode('utf-8') + ": " + str(num_of_make)
>
> This is a code smell, don't do this.
>
> Either move to all unicode strings internally or use all bytes and hope for the best. Why make needs this treatment now? I assume you get an unicode string from the API, correct?
>
>> +    print "\nGrand total: ", grand_total
>> +
>> +    # Second pass: Generate detailed report
>> +    print "\n\nDetailed information:"
>> +    print "---------------------\n"
>> +    for make in sorted(summaries):
>> +        print make.encode('utf-8') + ":"
>
> Same comment as above
>
>> +        num_of_make = 0
>> +        for model in sorted(summaries[make]):
>
> Same comment as in the other loop.
>
>> +            num_of_make += 1
>> +            print (" * " + model['model'] + " (CID " + model['cid'] +
>
> Please use str.format  instead
>
>> +                   "):")
>> +            print ("    https://certification.canonical.com/certificates/" +
>> +                   model['certnum'])
>> +        print make.encode('utf-8'), " TOTAL: ", num_of_make, "\n"
>
> Same comment, explicit .encode is a code smell
>
>> +
>> +
>> +if __name__ == "__main__":
>> +    sys.exit(main())
>>
>
>
> --
> https://code.launchpad.net/~rodsmith/hwcert-tools/get-ytd/+merge/253389
> You are reviewing the proposed merge of lp:~rodsmith/hwcert-tools/get-ytd into lp:~hardware-certification/hwcert-tools/reporting-tools.