Code review comment for lp:~j/loggerhead/loggerhead-add_url-prefix_to_serve-branches

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

A couple of quick comments:

9 + served_url = self._config.get_option('url_prefix') + name

I think it would be nice to run this through paste.request.resolve_relative_url() somewhere along the line.

11 + served_url = None

Changing served_url's default from loggerhead.apps.branch._DEFAULT to None will break things.

40 + help="All branches will be advertised to be available from this prefix")

This line's too long -- needs to be wrapped.

45 === modified file 'serve-branches.1'

Wow, you remembered the man page. I had forgotten it existed. \o/

review: Needs Fixing

« Back to merge proposal