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

On Wed, Mar 18, 2015 at 10:09 PM, Roderick Smith
<email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 03/18/2015 04:11 PM, Zygmunt Krynicki wrote:
>> Review: Needs Fixing
>>
>> Hey.
>>
>> Thanks for improving this, it looks much better. I must ask you to
>> run flake8 though as there are plenty of small things that flake
>> will tell you
> about.
>
> I'd done that before, but I hadn't had the flake8-docstrings
> installed. I've now installed it and it's coming up clean.
>
> A couple more comments below....
>
>>> + if certnum == "all": + request_params = {"username":
>>> username, + "api_key": api_key} +
>>> else: + request_params = {"username": username, +
>>> "api_key": api_key, + "name": certnum}
>>
>> Don't add empty lines within functions.
>
> Sorry, I must disagree on this one. From a cognitive point of view,
> blank lines help separate logically-related sections of functions,
> which helps both finding relevant sections of code and understanding
> it. In Googling the issue, I found, in PEP-8:
>
> : Use blank lines in functions, sparingly, to indicate logical
> : sections.
>
> Judging by comments on online forums, this sentence seems to be
> interpreted as anything from "use as many blank lines as you want" to
> "don't ever use blank lines." In the face of that ambiguity, I'm
> sticking with what both my personal preference and my cognitive
> psychology training says works.

Don't ever use two newlines. That's clearly wrong. I don't think this
trivial function warrants to have any newlines but if you want to keep
the single ones by all means do that.
>
>>> + "https://certification.canonical.com/me/") +
>>> parser.add_argument( + "release", help="Ubuntu release
>>> ('trusty', 'precise', etc.)")
>>
>> don't enumerate this but add choices=('trusty', ...) this will let
>> argparse check it automatically
>
> The downside to this is that it will require changes for each new
> version. Granted, that's not often, but I'd rather not deal with that
> (or, worse, foist it onto somebody who's never seen the code) when
> 16.04 or 18.04 comes out. As it is now, the script returns a report
> with no entries if the code name is mistyped, which IMHO is not a big
> deal; and it will work with future releases. Thus, I'd prefer to leave
> it as-is.

There's a python module with all known ubuntu releases but I forgot
how it's called. If you rather not do validation and rely on later
failures then that's okay too.

Thanks
ZK