Code review comment for lp:~tony-badwolf/quickly/modular_add

Revision history for this message
Michael Terry (mterry) wrote :

Neat, thanks Tony! This does seem easier to work with for the future.

77 + help_list = ['Add something to your project\n']
78 + for module in addable:
79 + try:
80 + help_list.append(getattr(store, module).help_text)
81 + except AttributeError:
82 + # ignore files in store that have no help for us
83 + pass
84 + help_text = ''.join(help_list)
85 + print _(help_text)

I would make this '\n\n\n'.join(help_list) and remove the surrounding endlines from the help strings in the store. This is more 'defensive' because it doesn't rely on the cooperation of items from the store.

Also, the translation call _() should go around the source strings (including the ones in the store), not the final "print help_text". Like so:
help_list = [_('Add something to your project')]

204 + templatetools.usage_error(_('Cannot add %s: it is not in the store' % argv[1]), cmd=cmd, template='ubuntu-application')

I would make this a little more user-friendly, like "Did not recognize %s" or something. A user isn't going to know what the store is.

23 +from store import *

Do you need this line?

24 +import store

It feels a bit odd using the store directory for this. What do you think about a new directory/submodule called 'add' or 'add_items' or something?

255 +abs_template_path = templatetools.get_template_path_from_project()

This line causes an error when I do this in the toplevel of the quickly branch due to loadConfig() eventually being called with can_stop=True:
PATH=./bin:$PATH quickly help ubuntu-application add

review: Needs Fixing

« Back to merge proposal