Comment on attachment 38904 [details] [review]
[SIGNED-OFF] Bug 14113: Silence t/Dates.t warnings and cleanup output.

Review of attachment 38904 [details] [review]:
-----------------------------------------------------------------

::: t/Dates.t
@@ +59,3 @@
>  foreach ( @{ $thash{'iso'} } ) {
> +    if ($_ =~ /1952/) {
> +        warning_like { $val = format_date($_); } qr/Illegal date/,

This is confusing I think.
The date is not illegal because it contains 1952, but because the day or month is 0.
Maybe should we test all cases (list all the illegal dates), or be explicit somewhere.

(In reply to Jonathan Druart from comment #3)
> Comment on attachment 38904 [details] [review] [review]
> [SIGNED-OFF] Bug 14113: Silence t/Dates.t warnings and cleanup output.
> 
> Review of attachment 38904 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: t/Dates.t
> @@ +59,3 @@
> >  foreach ( @{ $thash{'iso'} } ) {
> > +    if ($_ =~ /1952/) {
> > +        warning_like { $val = format_date($_); } qr/Illegal date/,
> 
> This is confusing I think.
> The date is not illegal because it contains 1952, but because the day or
> month is 0.

Yes, but of the dates in the hash, the only illegal dates have 1952 as the year.

> Maybe should we test all cases (list all the illegal dates), or be explicit
> somewhere.

I didn't want to restructure code. I suppose if we broke the dates into legal and illegal dates and did the same loop twice that would be okay?

(In reply to M. Tompsett from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Comment on attachment 38904 [details] [review] [review] [review]
> > [SIGNED-OFF] Bug 14113: Silence t/Dates.t warnings and cleanup output.
> > 
> > Review of attachment 38904 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: t/Dates.t
> > @@ +59,3 @@
> > >  foreach ( @{ $thash{'iso'} } ) {
> > > +    if ($_ =~ /1952/) {
> > > +        warning_like { $val = format_date($_); } qr/Illegal date/,
> > 
> > This is confusing I think.
> > The date is not illegal because it contains 1952, but because the day or
> > month is 0.
> 
> Yes, but of the dates in the hash, the only illegal dates have 1952 as the
> year.

Yes I understood, but it's confusing :)

> > Maybe should we test all cases (list all the illegal dates), or be explicit
> > somewhere.
> 
> I didn't want to restructure code. I suppose if we broke the dates into
> legal and illegal dates and did the same loop twice that would be okay?

What about checking /^00?-/, /-0?0$/ and /0{4}$/ ?

I still see the Illegal Date messages. Did you want to get rid of these or reword them entirely? If this is the expected outcome then I'm happy to sign off on this.

Created attachment 39695 [details] [review]
[PASSED QA] Bug 14113: Silence t/Dates.t warnings and cleanup output.

The expectation of a user not freaking out over 'Illegal date'
messages is not sensible. Catch the messages, give a nicer
message.

Also, compress the veritical spacing on the output.

TEST PLAN
---------
1) $ prove t/Dates.t
   -- notice Illegal date messages, and extra lines between
      some sectional messages.
2) Apply patch
3) $ prove t/Dates.t
   -- Notice how nice and clean it is?
      Try it with -v to see all the output, including the
      expected warning type messages.
4) run koha qa test tools.

Test Remarks : complies with test plan.
Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>a

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Patch pushed to master.

Thanks Mark!