Merge lp://staging/~mabac/launchpad-work-items-tracker/change-roadmap-urls into lp://staging/~linaro-automation/launchpad-work-items-tracker/linaro

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)

On Mon, Dec 12, 2011 at 6:04 PM, Guilherme Salgado
<email address hidden> wrote:
> Review: Approve
>
> 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.

Thanks! I went with the second option which supposedly is thread safe
and a little more efficient (only one call to the file system).
Admittedly it's just a theoretical difference for us, but it feels
better. ;)

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

I've fixed this one too, and the user and group mkdirs.

>
>> +
>>  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)
>
>
> --
> https://code.launchpad.net/~mabac/launchpad-work-items-tracker/change-roadmap-urls/+merge/85303
> You are the owner of lp:~mabac/launchpad-work-items-tracker/change-roadmap-urls.