launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

On 30/05/12 17:35, William Grant wrote:
> Review: Approve code
> 
> 137	+ grantee_ids = [grantee[0].id for grantee in grant_permissions]
> 
> This is not an entirely insignificant change: it causes grant_permissions to be iterated before the main loop. It looks like all callsites currently pass in a list, but if it happens to be a ResultSet then the pre-consumption will cause jsonShareeData to return an empty list. Hopefully not a concern here, though.
> 

Yes, this method is to be considered internal to the service and is
meant to be called with a list representing the current batch of results
to be rendered.

> 148	+ image_url = display_api.custom_icon_url() or ''
> 
> I think it feels slightly nicer to use None instead of ''. I'd also consider s/image/icon/.
> 

Ok. I'll make that change.


> 149	+ sprite_css = display_api.sprite_css() or 'sprite bullet'
> 
> Can sprite_css for a person or team ever return something that evaluates to false? This seems like over-nice handling of a case that should at best crash.

This code was taken from the TAL formatter but I guess we are always
guaranteed to be operating with a person or team so it can be simplified.

-- 
https://code.launchpad.net/~wallyworld/launchpad/sharee-display-details-1002954/+merge/107912
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.