Re: 1.524

On 25/03/13 14:28, H.Merijn Brand wrote:
> On Mon, 25 Mar 2013 13:55:26 +0000, "Martin J. Evans"
> <[email protected]> wrote:
>
>> On 25/03/13 11:51, Tim Bunce wrote:
>>> On Sun, Mar 24, 2013 at 09:08:33PM +0100, H.Merijn Brand wrote:
>>>> PASSes on all my boxes but one
>>>>
>>>> The failing box has JSON::XS installed (temporary to check Schmorp's
>>>> claims, for which I so far found no proof)
>>
>> What are those claims - is he claiming Perl has broken JSON::XS by any chance?
>
> If you have (a lot of) time, read
> https://rt.cpan.org/Public/Bug/Display.html?id=42462
> and
> https://rt.perl.org/rt3//Ticket/Display.html?id=117239

I don't have /that/ much time.

> The thing(s) I wanted to check are if the module is now 100% C89 safe
> (it is not) and if his claim that
>
>   #if defined(__BORLANDC__) || defined(_MSC_VER) || defined(__STDC__)
>             HE **hes = Perl_malloc (count * sizeof (HE *));
>   #else
>             HE *hes [count];
>   #endif
>
> will slowdown the code significantly on systems that do support C99

nc

>>>> The formatted output - I might have an output patch applied to make the
>>>> output style meet our requirements - causes this mismatch.
>>>
>>> Am I right in thinking that the goal of the JSON::XS test is to check
>>> whether JSON::XS puts quotes around values?
>>
>> It was a check to make sure that JSON::XS saw numbers in Perl as numbers
>> and not strings and so does not put quotes around them - the
>> DiscardString bit.
>>
>>> I'd suggest adding s/\s+//g, plus a comment, to make that explicit.
>>
>> Why are there extra spaces in the result - there are not when I run it.
>
> Because it is installed with a patch on this box to conform to the style
> we require. Obviously this is an old and incomplete patch, that I never
> bothered to fix as this module is prohibited in our company.
>
> What I was pointing at, is that I would most like not be the only one
> in the world to patch modules to behave as required, as not all modules
> have decent ways for configurating needs and - as this is open source -
> patching is allowed :)

People who change their modules to work differently from the ones on CPAN surely need to accept that their change might break other things. The test in question can work around it this time (because you've told us how you changed it) but what about when you make another change to modify the JSON::XS module. It seems like the test fails on this box because you changed JSON::XS and you want the DBI test suite to keep up with you.

Having said that white space is allowed in JSON but you might find yourself on a slippery slope as white space in quotes needs to be maintained.

> If, as both Tim and me already assumed, JSON::XS is just used to check
> quoting, s/\s+//g is a valid and safe fix.
>
>> Something else seems horribly broken here - admittedly I've not been
>> keeping up.
>
> broken? IMHO JSON::XS is very very broken when ran on anything other
> than GNU gcc supported platforms.

Merijn, I know your feelings on Marc Lehmann but I don't see they have any relevance here. Whether you like it or not, JSON::XS is the fastest JSON parser module for Perl that I am aware of (by all means correct me if you know better), probably the most featureful and one a lot of people (including myself) depend on. The test in question was added to check the changes I made for DiscardString and StrictlyTyped which I TOTALLY rely on as we are producing massive JSON files and every extra "" makes a big difference.

Martin

>>>> I don't mind when the verdict is: we cannot expect people to alter
>>>> whitespacing in module output, but having cpanprefs not only makes that
>>>> easy, but make installing a lot of modules suddenly become more logical
>>>> as I can "fix" the decisions the author made that I do not agree with.
>>>>
>>>> Personally I don't think prettied output checks in tests should ever
>>>> test on whitespace (including newlines) unless the output is completely
>>>> generated by the module itself or guaranteed by the module or its
>>>> documentation to not add or remove whitespace.
>>>>
>>>> The change to prevent this specific case is maybe somthing like
>>>> --8<---
>>>> --- a/t/90sql_type_cast.t 2013-03-24 21:00:02.167352360 +0100
>>>> +++ b/t/90sql_type_cast.t 2013-03-24 21:05:07.251376420 +0100
>>>> @@ -116,7 +116,7 @@ foreach my $test(@tests) {
>>>>                skip 'DiscardString not supported in PurePerl', 1
>>>>                    if $pp && ($test->[3] & DBIstcf_DISCARD_STRING);
>>>>
>>>> -            my $json = JSON::XS->new->encode([$val]);
>>>> +            (my $json = JSON::XS->new->encode([$val])) =~ s/\s+]$/]/;;
>>>>                #diag(neat($val), ",", $json);
>>>>                is($json, $test->[5], "json $test->[0]");
>>>>            };
>>>> -->8---
>>>>
>>>> but it doesn't catch changes that generate extra spaces output as
>>>> like "  [  99  ] "
>>>>
>>>> For me, the tests will pass again next week, as JSON::XS will be banned
>>>> to trash again
>>>>
>>>> t/87gofer_cache.t ............... ok
>>>> t/90sql_type_cast.t ............. 1/45
>>>> #   Failed test 'json undef'
>>>> #   at t/90sql_type_cast.t line 121.
>>>> #          got: '[null   ]'
>>>> #     expected: '[null]'
>>>>
>>>> #   Failed test 'json invalid sql type'
>>>> #   at t/90sql_type_cast.t line 121.
>>>> #          got: '["99"   ]'
>>>> #     expected: '["99"]'
>>>>
>>>> #   Failed test 'json non numeric cast to int'
>>>> #   at t/90sql_type_cast.t line 121.
>>>> #          got: '["aa"   ]'
>>>> #     expected: '["aa"]'
>>>>
>>>> #   Failed test 'json non numeric cast to int (strict)'
>>>> #   at t/90sql_type_cast.t line 121.
>>>> #          got: '["aa"   ]'
>>>> #     expected: '["aa"]'
>>>>
>>>> #   Failed test 'json small int cast to int'
>>>> #   at t/90sql_type_cast.t line 121.
>>>> #          got: '["99"   ]'
>>>> #     expected: '["99"]'
>>>>
>>>> #   Failed test 'json 2 byte max signed int cast to int'
>>>> #   at t/90sql_type_cast.t line 121.
>>>> #          got: '["32767"   ]'
>>>> #     expected: '["32767"]'
>>>>
>>>> #   Failed test 'json 2 byte max unsigned int cast to int'
>>>> #   at t/90sql_type_cast.t line 121.
>>>> #          got: '["65535"   ]'
>