Re: [Tarantool-patches] [tarantool-patches] [PATCH] json: improve error messages

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

On 06/12/2019 22:16, Roman Khabibov wrote:
> Hi! Thanks for the review.
> 
>> On Oct 17, 2019, at 00:45, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>> Hi! Thanks for the patch!
>>
>> Sorry, but I am not sure that now the messages are
>> better. Just look at it:
>>
>>    tarantool> json.decode('{')
>>    ---
>>    - error: Expected object key string but found '\0' on line 1 at character 2 here ^^^...
>>    ...
>>
>> What is `^^^`, what is `...`? In the ticket it is said, that a user should have a
>> context, but there is no a context.
>>
>> I was thinking, that the author of 4339 rather meant that ^^^ should be below the
>> error in an error string, like this:
>>
>>    tarantool> json.decode('{')
>>    ---
>>    - error: Expected object key string but found '\0' on line 1 at character 2 here:
>>             "{ "
>>              ^^^
>>    ...
>>
>> But I do realize, that it is too complex. And still ^^^ looks confusing.
>> It looks like arrows pointing up, while you are trying to put a
>> context after them.
>>
>> At least, you could make a context like this:
>>
>>    tarantool> json.decode('{')
>>    ---
>>    - error: Expected object key string but found '\0' on line 1 at character 2 here
>>             "{>>\0<<"
>>    ...
>>
>> So you would have arrows pointing right at the bad position.
>>
>> I tried an error not related to eof, and got the same confusing
>> result:
>>
>>    tarantool> json.decode('{1: "world"}')
>>    ---
>>    - error: 'Expected object key string but found int on line 1 at character 2 here ^^^:
>>        "world"}...'
>>    ...
>>
>> From that message looks like the error is in ':', or in "world", or
>> somewhere above because of up arrows.
> Let’s just remove “T_*” tokens. Let print of a context will be “wontfix". Double
> arrows isn’t very good, IMO, because we don’t know the length of wrong token.
> 

Sorry, but the context is a part of the ticket. The ticket
consists of 3 points:

  - Replace T_*;
  - Provide context;
  - Provide line number.

Line numbers already work. This patch removes T_*. But
the context still is not provided. Context is asked not
because it is necessary. But because it is really hard to
count character count, when it is big.

Please, add the context.

Talking of "don't know the length of a wrong token" - ok, then
you can provide just arrows pointing at the error from left,
if you don't know right end:

    tarantool> json.decode('{blablabla')
    ---
    - error: Expected object key string but found 'b' on line 1 at character 2 here
             "{ >> blabla..."
    ...

> @@ -154,4 +154,34 @@ tap.test("json", function(test)
>      _, err_msg = pcall(serializer.decode, '{"hello": "world",\n 100: 200}')
>      test:ok(string.find(err_msg, 'line 2 at character 2') ~= nil,
>              'mistake on second line')
> +
> +    --
> +    -- gh-3316: Make sure that tokens 'T_*' are  absent in error

Double whitespace after 'are'.