[tarantool-patches] Re: [PATCH v3] json: add options to json.encode()

Vladislav Shpilevoy v.shpilevoy at tarantool.org
Wed Aug 8 22:07:43 MSK 2018


Hi! Thanks for the fixes!

See below last (I hope) 3 comments.

> commit f6d41bdfcec241733f98b7d26715ece6625539aa
> Author: Roman Khabibov <roman.habibov1 at yandex.ru>
> Date:   Sun Jul 8 02:21:08 2018 +0300
> 
>      json: add options to json.encode()
>      
>      Add an ability to pass options to json.encode()/decode().
>      
>      Closes: #2888.

1. Put here a documentation bot request. We have a bot that tracks
all new commits and when in a one it sees a documentation request,
and the commit is pushed into the master, the bot opens an issue
in tarantool/doc repository. So the documentation writers team is
able to reflect your changes on site tarantool.io.

Your patch has changed public API and requires such documentation
request. For syntax see this:
https://github.com/tarantool/docbot#docbot---tarantool-documentation-pipeline-bot

For example of a request see this:
https://www.freelists.org/post/tarantool-patches/PATCH-78-box-introduce-boxctlpromote
where "@TarantoolBot document" is written.

For example of a result see this:
https://github.com/tarantool/doc/issues/created_by/TarantoolBot

Here you can check was your request successful or not (by syntax, for
instance): http://try.tarantool.org:11116

> 
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 2f0f4dcf8..3faeb416f 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -214,6 +214,48 @@ static struct {
>   	{ NULL, 0, 0, 0},
>   };
>   
> +/**
> + * Configure one field in @a cfg.
> + * @param L lua stack
> + * @param i index of option in OPTIONS[]
> + * @param cfg serializer to inherit configuration
> + * @retval pointer to the value of option
> + * @retval NULL if option is not> + * in the table

2. As I've already said, please, start a sentence from a capital letter
and finish with the dot. For example see other files:
https://github.com/tarantool/tarantool/blob/2.0/src/box/tuple.h#L367

In other places too.

> diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua
> index 42c79d6e9..f2c67ab0a 100755
> --- a/test/app-tap/json.test.lua
> +++ b/test/app-tap/json.test.lua
> @@ -22,7 +22,55 @@ end
>   
>   tap.test("json", function(test)
>       local serializer = require('json')
> -    test:plan(13)
> +    test:plan(25)
> +
> +-- gh-2888: Check the possibility of using options in encode()/decode().

3. From the previous review point 5 still is not fixed. Your comment
starts earlier than the function. But should be aligned by the function
body, like usual statement. (Add 4 spaces before the comment.)





More information about the Tarantool-patches mailing list