Code review comment for lp://staging/~rodsmith/hwcert-tools/get-ytd

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.