Code review comment for lp:~gary/launchpad/bug838878

Gary--

This looks good. I see one possible problem in the updated profile template, pointed out below.

> === modified file 'lib/lp/services/profile/profile.pt'
> --- lib/lp/services/profile/profile.pt 2011-08-22 17:08:02 +0000
> +++ lib/lp/services/profile/profile.pt 2011-09-01 17:17:32 +0000
> @@ -64,15 +64,21 @@
> get immediate profiling results in the browser, use
> <code>++profile++show</code>.</tal:block> <tal:block condition="not:
> always_log">In order to get profiling results, you need to ask for an HTML
> - view (<code>++profile++show</code>), a KCacheGrind-friendly log on the
> + view of the profile and OOPS data including SQL calls
> + (<code>++profile++show</code>), a KCacheGrind-friendly log on the
> filesystem (<code>++profile++callgrind</code>), a PStats-friendly log
> (Python standard library) on the filesystem
> - (<code>++profile++pstats</code>), or an HTML view of the SQL and the
> + (<code>++profile++pstats</code>), an HTML view of the SQL and the
> Python stacktrace when each SQL call was made
> - (<code>++profile++sqltrace</code>). You can also combine these
> - (<code>++profile++show,callgrind</code> or
> - <code>++profile++show,pstats</code> or others,
> - with each action separated with commas).</tal:block></p>
> + (<code>++profile++sqltrace</code>), or an HTML view of only the SQL
> + (<code>++profile++sql</code>). You can also combine these
> + (<code>++profile++show&callgrind</code> or
> + <code>++profile++show&pstats</code> or others, with each action separated
> + with commas).</tal:block></p>

I may be misunderstanding this, but shouldn't this be "separated with ambersands" as shown in the example?

> === modified file 'lib/lp/services/profile/profile.py'
> @@ -505,24 +521,108 @@
> # include traversal in the profile.
> actions, slash, tail = end.partition('/')
> result.update(
> - action for action in (
> - item.strip().lower() for item in actions.split(','))
> - if action)
> + (name.strip(), config.strip())
> + for name, partition, config in (
> + item.strip().lower().partition(':')
> + for item in actions.split('&'))
> + if name.strip())

There's no problem in the above bit, just thought that was one hell of a generator expression. :-P