Code review comment for lp:~michael.nelson/ubuntu-webcatalog/987838-no-screenshots-for-extras-2

Revision history for this message
Natalia Bidart (nataliabidart) wrote :

Branch look pretty awesome.

One tiny note, instead of:

if 'Screenshot-Url' in package.candidateRecord:
    screenshot_url = package.candidateRecord['Screenshot-Url']

I usually prefer something like:

screenshot_url = package.candidateRecord.get('Screenshot-Url')
if screenshot_url is not None and screenshot_url not in app.screenshots:
    app.applicationmedia_set.create(media_type='screenshot', url=screenshot_url)

The benefits I see in the second option are:

* you access the package.candidateRecord dict only once
* you do not duplicate the literal 'Screenshot-Url' and thus avoid risk of typos
* you avoid an if (and then another inner level in the logic)

So, if you think is worth it, feel free to tweak the code in this MP. I'll be approving anyways since this looks great.

review: Approve

« Back to merge proposal