Code review comment for lp:~kfogel/launchpad/515584-fix-DRY-violation

Revision history for this message
Eleanor Berger (intellectronica) wrote :

Hi Karl,

All-in-all this branch looks nice. I have one code-related comment: the property patchTaskOrderings should be called patch_task_orderings. Remember that even if its implementation is a method, from the interface perspective it looks just like another attribute, so it should follow the attribute naming convention.

As for the UI, I think there are two issues:

1. You're now capitalizing the sort orderings, which is different from the normal search sort order. I think that both should have the same capitalization, so I would prefer not to capitalize for now.

2. Where previously you displayed the target name (package, project) you now display 'Target', but I don't think this is a good idea, because nowhere else do we use the word target to mean what we mean here, and it will probably not be obvious to users what it means. So I think it's better to continue using targetName.

review: Needs Fixing

« Back to merge proposal