Code review comment for lp://staging/~mabac/launchpad-work-items-tracker/change-roadmap-urls

Hi Mattias,

This looks good to me; just one suggestion below.

 review approve

On Mon, 2011-12-12 at 09:32 +0000, Mattias Backman wrote:
[...]
> === modified file 'generate-all'
> --- generate-all 2011-12-07 09:03:27 +0000
> +++ generate-all 2011-12-12 09:28:04 +0000
> @@ -83,6 +83,18 @@
> except OSError:
> None
>
> +lanessubdir = os.path.join(opts.output_dir, '..', 'lane')
> +try:
> + os.mkdir(lanessubdir)
> +except OSError:
> + None

I think here we only want to ignore the error if the directory already
exists, right? In that case we should either check for its existence
before trying os.mkdir() or catch OSError but only ignore it if errno is
17 (file already exists). I think the former is clearer, but either is
fine, really.

> +
> +cardssubdir = os.path.join(opts.output_dir, '..', 'card')
> +try:
> + os.mkdir(cardssubdir)
> +except OSError:
> + None

Same here.

> +
> for u in users:
> for m in milestones:
> target = u + '-' + m
> @@ -156,7 +168,7 @@
>
> # roadmap lanes
> for lane in report_tools.lanes(store):
> - basename = os.path.join(opts.output_dir, 'roadmap-' + lane.name)
> + basename = os.path.join(lanessubdir, lane.name)
> report_tools.roadmap_pages(my_path, opts.database, basename, opts.config,
> lane, root=opts.root)
>
> @@ -165,8 +177,8 @@
> if card.roadmap_id != '':
> page_name = card.roadmap_id
> else:
> - page_name = card.card_id
> - basename = os.path.join(opts.output_dir, 'roadmap-card-%s' % page_name)
> + page_name = str(card.card_id)
> + basename = os.path.join(cardssubdir, page_name)
> report_tools.roadmap_cards(my_path, opts.database, basename, opts.config,
> card, root=opts.root)